| Message ID | 20240815093715.165305-1-dev@lankhorst.se |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Maarten, Thank you for highlighting this. I'm curious that it hasn't come up in our builds yet! What version of clang are you using? Quoting Maarten Lankhorst (2024-08-15 10:37:15) > I was getting the following error in clang: > ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] > if (abs(denominator) > eps) { > ^ > ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead Is there a preference against the recommended std::abs? > if (abs(denominator) > eps) { > ^~~ > std::abs > > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> > --- > src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index f45525bc..c5750c84 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con > const double eps = 1e-3; > ipa::Pwl::Point ca = c - a, ba = b - a; > double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); > - if (abs(denominator) > eps) { > + if (fabs(denominator) > eps) { > double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); > double result = numerator / denominator + a.x(); > return std::max(a.x(), std::min(c.x(), result)); > -- > 2.39.2 >
On Thu, Aug 15, 2024 at 10:51:38AM +0100, Kieran Bingham wrote: > Hi Maarten, > > Thank you for highlighting this. I'm curious that it hasn't come up in > our builds yet! What version of clang are you using? > > Quoting Maarten Lankhorst (2024-08-15 10:37:15) > > I was getting the following error in clang: > > ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] > > if (abs(denominator) > eps) { > > ^ > > ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead > > Is there a preference against the recommended std::abs? Documentation/coding-style.rst states 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. There's also a set of calls to abs() abd fabs() in src/ipa/rpi/controller/rpi/alsc.cpp which should be replaced by std::abs(). Maarten, would you like to submit a new version that addresses both ? > > if (abs(denominator) > eps) { > > ^~~ > > std::abs > > > > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> > > --- > > src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index f45525bc..c5750c84 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con > > const double eps = 1e-3; > > ipa::Pwl::Point ca = c - a, ba = b - a; > > double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); > > - if (abs(denominator) > eps) { > > + if (fabs(denominator) > eps) { > > double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); > > double result = numerator / denominator + a.x(); > > return std::max(a.x(), std::min(c.x(), result));
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Hi Maarten, > > Thank you for highlighting this. I'm curious that it hasn't come up in > our builds yet! What version of clang are you using? It happens to me too, with clang 18.1.6 on Fedora 40. > Quoting Maarten Lankhorst (2024-08-15 10:37:15) >> I was getting the following error in clang: >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] >> if (abs(denominator) > eps) { >> ^ >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead > > Is there a preference against the recommended std::abs? > >> if (abs(denominator) > eps) { >> ^~~ >> std::abs >> >> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> >> --- >> src/ipa/rpi/controller/rpi/awb.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp >> index f45525bc..c5750c84 100644 >> --- a/src/ipa/rpi/controller/rpi/awb.cpp >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp >> @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con >> const double eps = 1e-3; >> ipa::Pwl::Point ca = c - a, ba = b - a; >> double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); >> - if (abs(denominator) > eps) { >> + if (fabs(denominator) > eps) { >> double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); >> double result = numerator / denominator + a.x(); >> return std::max(a.x(), std::min(c.x(), result)); >> -- >> 2.39.2 >>
On Fri, Aug 16, 2024 at 03:40:45PM +0200, Milan Zamazal wrote: > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Hi Maarten, > > > > Thank you for highlighting this. I'm curious that it hasn't come up in > > our builds yet! What version of clang are you using? > > It happens to me too, with clang 18.1.6 on Fedora 40. I'm very surprised, I tested with clang 18.1.8 locally and didn't see any error. I would have fixed it already otherwise :-) > > Quoting Maarten Lankhorst (2024-08-15 10:37:15) > >> I was getting the following error in clang: > >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] > >> if (abs(denominator) > eps) { > >> ^ > >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead > > > > Is there a preference against the recommended std::abs? > > > >> if (abs(denominator) > eps) { > >> ^~~ > >> std::abs > >> > >> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> > >> --- > >> src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > >> index f45525bc..c5750c84 100644 > >> --- a/src/ipa/rpi/controller/rpi/awb.cpp > >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp > >> @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con > >> const double eps = 1e-3; > >> ipa::Pwl::Point ca = c - a, ba = b - a; > >> double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); > >> - if (abs(denominator) > eps) { > >> + if (fabs(denominator) > eps) { > >> double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); > >> double result = numerator / denominator + a.x(); > >> return std::max(a.x(), std::min(c.x(), result));
On Fri, Aug 16, 2024 at 05:12:28PM +0200, Milan Zamazal wrote: > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > > On Fri, Aug 16, 2024 at 03:40:45PM +0200, Milan Zamazal wrote: > >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > >> > >> > Hi Maarten, > >> > > >> > Thank you for highlighting this. I'm curious that it hasn't come up in > >> > our builds yet! What version of clang are you using? > >> > >> It happens to me too, with clang 18.1.6 on Fedora 40. > > > > I'm very surprised, I tested with clang 18.1.8 locally and didn't see > > any error. I would have fixed it already otherwise :-) > > Interesting. I checked in another environment with clang 17.0.6 and it > also reports the error. Can it perhaps depend on particular C/C++ > header setup/version? When I ask LSP about `abs' at the given place, > it points me to the following from stdlib.h: > > extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur; Well regardless of where the problem comes from, I think we should fix it. I'll review a v2 that uses std::abs(), and ideally addresses alsc.cpp too. > >> > Quoting Maarten Lankhorst (2024-08-15 10:37:15) > >> >> I was getting the following error in clang: > >> >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] > >> >> if (abs(denominator) > eps) { > >> >> ^ > >> >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead > >> > > >> > Is there a preference against the recommended std::abs? > >> > > >> >> if (abs(denominator) > eps) { > >> >> ^~~ > >> >> std::abs > >> >> > >> >> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> > >> >> --- > >> >> src/ipa/rpi/controller/rpi/awb.cpp | 2 +- > >> >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > >> >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > >> >> index f45525bc..c5750c84 100644 > >> >> --- a/src/ipa/rpi/controller/rpi/awb.cpp > >> >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp > >> >> @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con > >> >> const double eps = 1e-3; > >> >> ipa::Pwl::Point ca = c - a, ba = b - a; > >> >> double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); > >> >> - if (abs(denominator) > eps) { > >> >> + if (fabs(denominator) > eps) { > >> >> double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); > >> >> double result = numerator / denominator + a.x(); > >> >> return std::max(a.x(), std::min(c.x(), result));
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Fri, Aug 16, 2024 at 05:12:28PM +0200, Milan Zamazal wrote: >> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: >> > On Fri, Aug 16, 2024 at 03:40:45PM +0200, Milan Zamazal wrote: > >> >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >> >> >> > Hi Maarten, >> >> > >> >> > Thank you for highlighting this. I'm curious that it hasn't come up in >> >> > our builds yet! What version of clang are you using? >> >> >> >> It happens to me too, with clang 18.1.6 on Fedora 40. >> > >> > I'm very surprised, I tested with clang 18.1.8 locally and didn't see >> > any error. I would have fixed it already otherwise :-) >> >> Interesting. I checked in another environment with clang 17.0.6 and it >> also reports the error. Can it perhaps depend on particular C/C++ >> header setup/version? When I ask LSP about `abs' at the given place, >> it points me to the following from stdlib.h: >> >> extern int abs (int __x) __THROW __attribute__ ((__const__)) __wur; > > Well regardless of where the problem comes from, I think we should fix > it. Definitely. But it would be interesting to know why it hasn't been caught by the CI. > I'll review a v2 that uses std::abs(), and ideally addresses alsc.cpp > too. Thank you. >> >> > Quoting Maarten Lankhorst (2024-08-15 10:37:15) >> >> >> I was getting the following error in clang: >> >> >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] >> >> >> if (abs(denominator) > eps) { >> >> >> ^ >> >> >> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead >> >> > >> >> > Is there a preference against the recommended std::abs? >> >> > >> >> >> if (abs(denominator) > eps) { >> >> >> ^~~ >> >> >> std::abs >> >> >> >> >> >> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> >> >> >> --- >> >> >> src/ipa/rpi/controller/rpi/awb.cpp | 2 +- >> >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> >> >> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp >> >> >> index f45525bc..c5750c84 100644 >> >> >> --- a/src/ipa/rpi/controller/rpi/awb.cpp >> >> >> +++ b/src/ipa/rpi/controller/rpi/awb.cpp >> >> >> @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con >> >> >> const double eps = 1e-3; >> >> >> ipa::Pwl::Point ca = c - a, ba = b - a; >> >> >> double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); >> >> >> - if (abs(denominator) > eps) { >> >> >> + if (fabs(denominator) > eps) { >> >> >> double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); >> >> >> double result = numerator / denominator + a.x(); >> >> >> return std::max(a.x(), std::min(c.x(), result));
diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp index f45525bc..c5750c84 100644 --- a/src/ipa/rpi/controller/rpi/awb.cpp +++ b/src/ipa/rpi/controller/rpi/awb.cpp @@ -505,7 +505,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con const double eps = 1e-3; ipa::Pwl::Point ca = c - a, ba = b - a; double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x()); - if (abs(denominator) > eps) { + if (fabs(denominator) > eps) { double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x(); double result = numerator / denominator + a.x(); return std::max(a.x(), std::min(c.x(), result));
I was getting the following error in clang: ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value] if (abs(denominator) > eps) { ^ ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead if (abs(denominator) > eps) { ^~~ std::abs Signed-off-by: Maarten Lankhorst <dev@lankhorst.se> --- src/ipa/rpi/controller/rpi/awb.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)