[libcamera-devel,v5,06/10] ipa: raspberry: replace abs() with std::abs()
diff mbox series

Message ID 20221028031726.4849-7-nicholas@rothemail.net
State Accepted
Headers show
Series
  • [libcamera-devel,v5,01/10] ipa: workaround libcxx duration limitation
Related show

Commit Message

Nicolas Dufresne via libcamera-devel Oct. 28, 2022, 3:17 a.m. UTC 
From: Nicholas Roth <nicholas@rothemail.net>

pwl.cpp uses abs() instead of std::abs(), which causes unexpected
behavior in the Clang compiler used for Android. Replace with
C++-standard absolute value function std::abs(), which supports
double-precision absolute values in a standard way.

Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
---
 src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Kieran Bingham Oct. 28, 2022, 10:27 a.m. UTC | #1 
Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> From: Nicholas Roth <nicholas@rothemail.net>
> 
> pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> behavior in the Clang compiler used for Android. Replace with
> C++-standard absolute value function std::abs(), which supports
> double-precision absolute values in a standard way.
> 
> Signed-off-by: Nicholas Roth <nicholas@rothemail.net>

From Jacopo on v1:

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

And from me:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Though we really might have to do a bit of extra validation here:

 https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say

Seems a bit worrying.

Naush - any idea here? 


> ---
>  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/pwl.cpp b/src/ipa/raspberrypi/controller/pwl.cpp
> index c59f5fa1..70c2e24b 100644
> --- a/src/ipa/raspberrypi/controller/pwl.cpp
> +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <cassert>
> +#include <cmath>
>  #include <stdexcept>
>  
>  #include "pwl.h"
> @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
>         while (thisSpan != (int)points_.size() - 1) {
>                 double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
>                        dy = points_[thisSpan + 1].y - points_[thisSpan].y;
> -               if (abs(dy) > eps &&
> +               if (std::abs(dy) > eps &&
>                     otherSpan + 1 < (int)other.points_.size() &&
>                     points_[thisSpan + 1].y >=
>                             other.points_[otherSpan + 1].x + eps) {
> @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
>                                  points_[thisSpan].y) *
>                                         dx / dy;
>                         thisY = other.points_[++otherSpan].x;
> -               } else if (abs(dy) > eps && otherSpan > 0 &&
> +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
>                            points_[thisSpan + 1].y <=
>                                    other.points_[otherSpan - 1].x - eps) {
>                         /*
> -- 
> 2.34.1
>
Naushir Patuck Oct. 28, 2022, 10:40 a.m. UTC | #2 
On Fri, 28 Oct 2022 at 11:27, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > From: Nicholas Roth <nicholas@rothemail.net>
> >
> > pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> > behavior in the Clang compiler used for Android. Replace with
> > C++-standard absolute value function std::abs(), which supports
> > double-precision absolute values in a standard way.
> >
> > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
>
> From Jacopo on v1:
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> And from me:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Though we really might have to do a bit of extra validation here:
>
>
> https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say
>
> Seems a bit worrying.
>
> Naush - any idea here?


I think this code change ought to be safe - we are including cmath header,
and dy is a double,
so the compiler uses the double abs(double arg) signature.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


>
>
>
> > ---
> >  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> b/src/ipa/raspberrypi/controller/pwl.cpp
> > index c59f5fa1..70c2e24b 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <cassert>
> > +#include <cmath>
> >  #include <stdexcept>
> >
> >  #include "pwl.h"
> > @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >         while (thisSpan != (int)points_.size() - 1) {
> >                 double dx = points_[thisSpan + 1].x -
> points_[thisSpan].x,
> >                        dy = points_[thisSpan + 1].y -
> points_[thisSpan].y;
> > -               if (abs(dy) > eps &&
> > +               if (std::abs(dy) > eps &&
> >                     otherSpan + 1 < (int)other.points_.size() &&
> >                     points_[thisSpan + 1].y >=
> >                             other.points_[otherSpan + 1].x + eps) {
> > @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >                                  points_[thisSpan].y) *
> >                                         dx / dy;
> >                         thisY = other.points_[++otherSpan].x;
> > -               } else if (abs(dy) > eps && otherSpan > 0 &&
> > +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
> >                            points_[thisSpan + 1].y <=
> >                                    other.points_[otherSpan - 1].x - eps)
> {
> >                         /*
> > --
> > 2.34.1
> >
>

On Fri, 28 Oct 2022 at 11:27, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > From: Nicholas Roth <nicholas@rothemail.net>
> >
> > pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> > behavior in the Clang compiler used for Android. Replace with
> > C++-standard absolute value function std::abs(), which supports
> > double-precision absolute values in a standard way.
> >
> > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
>
> From Jacopo on v1:
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> And from me:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Though we really might have to do a bit of extra validation here:
>
>
> https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say
>
> Seems a bit worrying.
>
> Naush - any idea here?
>
>
> > ---
> >  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> b/src/ipa/raspberrypi/controller/pwl.cpp
> > index c59f5fa1..70c2e24b 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <cassert>
> > +#include <cmath>
> >  #include <stdexcept>
> >
> >  #include "pwl.h"
> > @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >         while (thisSpan != (int)points_.size() - 1) {
> >                 double dx = points_[thisSpan + 1].x -
> points_[thisSpan].x,
> >                        dy = points_[thisSpan + 1].y -
> points_[thisSpan].y;
> > -               if (abs(dy) > eps &&
> > +               if (std::abs(dy) > eps &&
> >                     otherSpan + 1 < (int)other.points_.size() &&
> >                     points_[thisSpan + 1].y >=
> >                             other.points_[otherSpan + 1].x + eps) {
> > @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >                                  points_[thisSpan].y) *
> >                                         dx / dy;
> >                         thisY = other.points_[++otherSpan].x;
> > -               } else if (abs(dy) > eps && otherSpan > 0 &&
> > +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
> >                            points_[thisSpan + 1].y <=
> >                                    other.points_[otherSpan - 1].x - eps)
> {
> >                         /*
> > --
> > 2.34.1
> >
>
Jacopo Mondi Oct. 28, 2022, 11:07 a.m. UTC | #3 
Hi Kieran

On Fri, Oct 28, 2022 at 11:40:50AM +0100, Naushir Patuck via libcamera-devel wrote:
> On Fri, 28 Oct 2022 at 11:27, Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
>
> > Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > > From: Nicholas Roth <nicholas@rothemail.net>
> > >
> > > pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> > > behavior in the Clang compiler used for Android. Replace with
> > > C++-standard absolute value function std::abs(), which supports
> > > double-precision absolute values in a standard way.
> > >
> > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> >
> > From Jacopo on v1:
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > And from me:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Though we really might have to do a bit of extra validation here:
> >
> >
> > https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say
> >
> > Seems a bit worrying.
> >
> > Naush - any idea here?

The change matches our suggested coding practices

from Documentation/coding-style.rst:

Usage of the C compatibility headers is preferred, except for the math.h header.
Where math.h defines separate functions for different argument types (e.g.
abs(int), labs(long int), fabs(double) and fabsf(float)) and requires the
developer to pick the right function, cmath defines overloaded functions
(std::abs(int), std::abs(long int), std::abs(double) and std::abs(float) to let
the compiler select the right function. This avoids potential errors such as
calling abs(int) with a float argument, performing an unwanted implicit integer
conversion. For this reason, cmath is preferred over math.h.


>
>
> I think this code change ought to be safe - we are including cmath header,
> and dy is a double,
> so the compiler uses the double abs(double arg) signature.
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
>
> >
> >
> >
> > > ---
> > >  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> > b/src/ipa/raspberrypi/controller/pwl.cpp
> > > index c59f5fa1..70c2e24b 100644
> > > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > > @@ -6,6 +6,7 @@
> > >   */
> > >
> > >  #include <cassert>
> > > +#include <cmath>
> > >  #include <stdexcept>
> > >
> > >  #include "pwl.h"
> > > @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> > const
> > >         while (thisSpan != (int)points_.size() - 1) {
> > >                 double dx = points_[thisSpan + 1].x -
> > points_[thisSpan].x,
> > >                        dy = points_[thisSpan + 1].y -
> > points_[thisSpan].y;
> > > -               if (abs(dy) > eps &&
> > > +               if (std::abs(dy) > eps &&
> > >                     otherSpan + 1 < (int)other.points_.size() &&
> > >                     points_[thisSpan + 1].y >=
> > >                             other.points_[otherSpan + 1].x + eps) {
> > > @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> > const
> > >                                  points_[thisSpan].y) *
> > >                                         dx / dy;
> > >                         thisY = other.points_[++otherSpan].x;
> > > -               } else if (abs(dy) > eps && otherSpan > 0 &&
> > > +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
> > >                            points_[thisSpan + 1].y <=
> > >                                    other.points_[otherSpan - 1].x - eps)
> > {
> > >                         /*
> > > --
> > > 2.34.1
> > >
> >
>
> On Fri, 28 Oct 2022 at 11:27, Kieran Bingham <
> kieran.bingham@ideasonboard.com> wrote:
>
> > Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > > From: Nicholas Roth <nicholas@rothemail.net>
> > >
> > > pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> > > behavior in the Clang compiler used for Android. Replace with
> > > C++-standard absolute value function std::abs(), which supports
> > > double-precision absolute values in a standard way.
> > >
> > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> >
> > From Jacopo on v1:
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > And from me:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Though we really might have to do a bit of extra validation here:
> >
> >
> > https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say
> >
> > Seems a bit worrying.
> >
> > Naush - any idea here?
> >
> >
> > > ---
> > >  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> > b/src/ipa/raspberrypi/controller/pwl.cpp
> > > index c59f5fa1..70c2e24b 100644
> > > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > > @@ -6,6 +6,7 @@
> > >   */
> > >
> > >  #include <cassert>
> > > +#include <cmath>
> > >  #include <stdexcept>
> > >
> > >  #include "pwl.h"
> > > @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> > const
> > >         while (thisSpan != (int)points_.size() - 1) {
> > >                 double dx = points_[thisSpan + 1].x -
> > points_[thisSpan].x,
> > >                        dy = points_[thisSpan + 1].y -
> > points_[thisSpan].y;
> > > -               if (abs(dy) > eps &&
> > > +               if (std::abs(dy) > eps &&
> > >                     otherSpan + 1 < (int)other.points_.size() &&
> > >                     points_[thisSpan + 1].y >=
> > >                             other.points_[otherSpan + 1].x + eps) {
> > > @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> > const
> > >                                  points_[thisSpan].y) *
> > >                                         dx / dy;
> > >                         thisY = other.points_[++otherSpan].x;
> > > -               } else if (abs(dy) > eps && otherSpan > 0 &&
> > > +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
> > >                            points_[thisSpan + 1].y <=
> > >                                    other.points_[otherSpan - 1].x - eps)
> > {
> > >                         /*
> > > --
> > > 2.34.1
> > >
> >

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/controller/pwl.cpp b/src/ipa/raspberrypi/controller/pwl.cpp
index c59f5fa1..70c2e24b 100644
--- a/src/ipa/raspberrypi/controller/pwl.cpp
+++ b/src/ipa/raspberrypi/controller/pwl.cpp
@@ -6,6 +6,7 @@ 
  */
 
 #include <cassert>
+#include <cmath>
 #include <stdexcept>
 
 #include "pwl.h"
@@ -168,7 +169,7 @@  Pwl Pwl::compose(Pwl const &other, const double eps) const
 	while (thisSpan != (int)points_.size() - 1) {
 		double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
 		       dy = points_[thisSpan + 1].y - points_[thisSpan].y;
-		if (abs(dy) > eps &&
+		if (std::abs(dy) > eps &&
 		    otherSpan + 1 < (int)other.points_.size() &&
 		    points_[thisSpan + 1].y >=
 			    other.points_[otherSpan + 1].x + eps) {
@@ -181,7 +182,7 @@  Pwl Pwl::compose(Pwl const &other, const double eps) const
 				 points_[thisSpan].y) *
 					dx / dy;
 			thisY = other.points_[++otherSpan].x;
-		} else if (abs(dy) > eps && otherSpan > 0 &&
+		} else if (std::abs(dy) > eps && otherSpan > 0 &&
 			   points_[thisSpan + 1].y <=
 				   other.points_[otherSpan - 1].x - eps) {
 			/*