[libcamera-devel] [PATCH v5 06/10] ipa: raspberry: replace abs() with std::abs()

[Jacopo Mondi]

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 at ideasonboard.com> wrote:
>
> > Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > > From: Nicholas Roth <nicholas at 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 at rothemail.net>
> >
> > From Jacopo on v1:
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > And from me:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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 at 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 at ideasonboard.com> wrote:
>
> > Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > > From: Nicholas Roth <nicholas at 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 at rothemail.net>
> >
> > From Jacopo on v1:
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > And from me:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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
> > >
> >