[libcamera-devel] libcamera: ipu3: Fix compilation issues with gcc5

9153 diff mbox series

GCC5 does not provide prototypes for the math library functions defined
in the math.h header for the std:: namespace.

Include the C++ <cmath> header in place of <math.h> as it defines
overloads for the std::abs and std::fmod function.

This goes intentionally against the libcamera coding guidelines, and
is reported as warning by checkpatch.py.

Fixes: 968ab9bad0ed ("libcamera: ipu3: imgu: Calculate ImgU pipe configuration")
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Jacopo,

Thank you for the patch.

On Mon, Aug 03, 2020 at 06:28:11PM +0200, Jacopo Mondi wrote:
> GCC5 does not provide prototypes for the math library functions defined
> in the math.h header for the std:: namespace.
> 
> Include the C++ <cmath> header in place of <math.h> as it defines
> overloads for the std::abs and std::fmod function.
> 
> This goes intentionally against the libcamera coding guidelines, and
> is reported as warning by checkpatch.py.

I'm considering adjusting the guidelines specifically for cmath, as the
math function overloads are really nice to have, and should do the right
thing by default.

> Fixes: 968ab9bad0ed ("libcamera: ipu3: imgu: Calculate ImgU pipe configuration")
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Please push :-)

> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index b7593ceb3672..eb829e096561 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -7,8 +7,8 @@
>  
>  #include "imgu.h"
>  
> +#include <cmath>
>  #include <limits>
> -#include <math.h>
>  
>  #include <linux/media-bus-format.h>
>

Hi Laurent,

On Wed, Aug 05, 2020 at 02:00:34AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Aug 03, 2020 at 06:28:11PM +0200, Jacopo Mondi wrote:
> > GCC5 does not provide prototypes for the math library functions defined
> > in the math.h header for the std:: namespace.
> >
> > Include the C++ <cmath> header in place of <math.h> as it defines
> > overloads for the std::abs and std::fmod function.
> >
> > This goes intentionally against the libcamera coding guidelines, and
> > is reported as warning by checkpatch.py.
>
> I'm considering adjusting the guidelines specifically for cmath, as the
> math function overloads are really nice to have, and should do the right
> thing by default.

What about

--- a/Documentation/coding-style.rst
+++ b/Documentation/coding-style.rst
@@ -198,7 +198,14 @@ std namespace, and may declare the same names in the global namespace. The C
 compatibility headers declare names in the global namespace, and may declare
 the same names in the std namespace. Usage of the C compatibility headers is
 strongly preferred. Code shall not rely on the optional declaration of names in
-the global or std namespace.
+the global or std namespace. An exception to this rule is represented by the
+[cmath|math.h] header files. As the type of the argument is particularly
+relevant for mathematical operations, the former (cmath) defines in the std
+namespace overloads of the same function that accept different parameter types,
+while the latter (math.h) requires the developer to pick the right function for
+the function arguments at hand. As an example, cmath defines std::abs(int) and
+std::abs(float) while math.h defines abs(int) and fabs(float). For this reason
+the inclusion of <cmath> is preferred over <math.h>.

>
> > Fixes: 968ab9bad0ed ("libcamera: ipu3: imgu: Calculate ImgU pipe configuration")
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Please push :-)

Thanks
   j
>
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index b7593ceb3672..eb829e096561 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -7,8 +7,8 @@
> >
> >  #include "imgu.h"
> >
> > +#include <cmath>
> >  #include <limits>
> > -#include <math.h>
> >
> >  #include <linux/media-bus-format.h>
> >
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Aug 05, 2020 at 09:10:29AM +0200, Jacopo Mondi wrote:
> On Wed, Aug 05, 2020 at 02:00:34AM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 03, 2020 at 06:28:11PM +0200, Jacopo Mondi wrote:
> > > GCC5 does not provide prototypes for the math library functions defined
> > > in the math.h header for the std:: namespace.
> > >
> > > Include the C++ <cmath> header in place of <math.h> as it defines
> > > overloads for the std::abs and std::fmod function.
> > >
> > > This goes intentionally against the libcamera coding guidelines, and
> > > is reported as warning by checkpatch.py.
> >
> > I'm considering adjusting the guidelines specifically for cmath, as the
> > math function overloads are really nice to have, and should do the right
> > thing by default.
> 
> What about
> 
> --- a/Documentation/coding-style.rst
> +++ b/Documentation/coding-style.rst
> @@ -198,7 +198,14 @@ std namespace, and may declare the same names in the global namespace. The C
>  compatibility headers declare names in the global namespace, and may declare
>  the same names in the std namespace. Usage of the C compatibility headers is
>  strongly preferred. Code shall not rely on the optional declaration of names in
> -the global or std namespace.
> +the global or std namespace. An exception to this rule is represented by the
> +[cmath|math.h] header files. As the type of the argument is particularly
> +relevant for mathematical operations, the former (cmath) defines in the std
> +namespace overloads of the same function that accept different parameter types,
> +while the latter (math.h) requires the developer to pick the right function for
> +the function arguments at hand. As an example, cmath defines std::abs(int) and
> +std::abs(float) while math.h defines abs(int) and fabs(float). For this reason
> +the inclusion of <cmath> is preferred over <math.h>.

I'd split this in two paragraphs, with the first one being the existing
text minus the penultimate sentence. Code shall not rely on the optional
declarations in any case, there's no exception to that rule. It could
become


 <cxxx> while the later are named <xxx.h>. The C++ headers declare names in the
 std namespace, and may declare the same names in the global namespace. The C
 compatibility headers declare names in the global namespace, and may declare
-the same names in the std namespace. Usage of the C compatibility headers is
-strongly preferred. Code shall not rely on the optional declaration of names in
-the global or std namespace.
+the same names in the std namespace. Code shall not rely on the optional
+declaration of names in the global or std namespace.
+
+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) and 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.


And we should also add

diff --git a/utils/checkstyle.py b/utils/checkstyle.py
index b594a19aed5b..d5dc26c0f666 100755
--- a/utils/checkstyle.py
+++ b/utils/checkstyle.py
@@ -244,9 +244,9 @@ class IncludeChecker(StyleChecker):
     patterns = ('*.cpp', '*.h')

     headers = ('assert', 'ctype', 'errno', 'fenv', 'float', 'inttypes',
-               'limits', 'locale', 'math', 'setjmp', 'signal', 'stdarg',
-               'stddef', 'stdint', 'stdio', 'stdlib', 'string', 'time', 'uchar',
-               'wchar', 'wctype')
+               'limits', 'locale', 'setjmp', 'signal', 'stdarg', 'stddef',
+               'stdint', 'stdio', 'stdlib', 'string', 'time', 'uchar', 'wchar',
+               'wctype')
     include_regex = re.compile('^#include <c([a-z]*)>')

     def __init__(self, content):

> > > Fixes: 968ab9bad0ed ("libcamera: ipu3: imgu: Calculate ImgU pipe configuration")
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Please push :-)
> 
> Thanks
>
> > > ---
> > >  src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > index b7593ceb3672..eb829e096561 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > @@ -7,8 +7,8 @@
> > >
> > >  #include "imgu.h"
> > >
> > > +#include <cmath>
> > >  #include <limits>
> > > -#include <math.h>
> > >
> > >  #include <linux/media-bus-format.h>
> > >