[libcamera-devel,v1.1,1/5] meson: Switch to C++17

9372 diff mbox series

Due to popular request, move from C++14 to C++17. This will allow
dropping some custom constructs (such as a custom utils::clamp),
benefiting from new language features, and dropping gcc 5 and 6 from the
compilation tests.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Update documentation
- Print an error message when detecting a too old compiler
---
 Documentation/coding-style.rst |  5 ++---
 meson.build                    | 29 ++++++++++++++++++-----------
 2 files changed, 20 insertions(+), 14 deletions(-)

On 22/08/2020 21:04, Laurent Pinchart wrote:
> Due to popular request, move from C++14 to C++17. This will allow
> dropping some custom constructs (such as a custom utils::clamp),
> benefiting from new language features, and dropping gcc 5 and 6 from the
> compilation tests.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Update documentation
> - Print an error message when detecting a too old compiler

Ok, some useful additions... reading. ..


> ---
>  Documentation/coding-style.rst |  5 ++---
>  meson.build                    | 29 ++++++++++++++++++-----------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> index 7acba37b8de8..7c56a1b70014 100644
> --- a/Documentation/coding-style.rst
> +++ b/Documentation/coding-style.rst
> @@ -88,13 +88,12 @@ headers, and with double quotes for other libcamera headers.
>  C++ Specific Rules
>  ------------------
>  
> -The code shall be implemented in C++14, with the following caveats:
> +The code shall be implemented in C++17, with the following caveats:
>  
>  * Type inference (auto and decltype) shall be used with caution, to avoid
>    drifting towards an untyped language.
>  * The explicit, override and final specifiers are to be used where applicable.
> -* General-purpose smart pointers (std::unique_ptr) deprecate std::auto_ptr.
> -  Smart pointers, as well as shared pointers and weak pointers, shall not be
> +* Smart pointers, as well as shared pointers and weak pointers, shall not be
>    overused.
>  * Classes are encouraged to define move constructors and assignment operators
>    where applicable, and generally make use of the features offered by rvalue
> diff --git a/meson.build b/meson.build
> index ec54e68f3635..cf2636d97100 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -6,7 +6,7 @@ project('libcamera', 'c', 'cpp',
>      default_options : [
>          'werror=true',
>          'warning_level=2',
> -        'cpp_std=c++14',
> +        'cpp_std=c++17',
>      ],
>      license : 'LGPL 2.1+')
>  
> @@ -62,6 +62,23 @@ if cc.get_id() == 'clang'
>      endif
>  endif
>  
> +if cc.get_id() == 'gcc'
> +    if cc.version().version_compare('<7')
> +        error('gcc version is too old, libcamera requires 7.0 or newer')
> +    endif


Clang v5 or greater is required for C++17. Do we need to catch that too?
or is that considered old enough that we're not going to hit it.

I think it looks like Clang 5 was released around September 2017, while
GCC 7.1 was released earlier in May 2017.

That said, wouldn't the compiler warn us quite verbosely if it didn't
support C++17 ?

I.e. .. do /we/ really need to do this check here in meson?

Anyway,

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

Still stands.


> +
> +    # gcc 7.1 introduced processor-specific ABI breakages related to parameter
> +    # passing on ARM platforms. This generates a large number of messages
> +    # during compilation with gcc >=7.1 until gcc 9. Silence them.
> +    if (host_machine.cpu_family() == 'arm' and
> +        cc.version().version_compare('>=7.1') and
> +        cc.version().version_compare('<9'))
> +        cpp_arguments += [
> +            '-Wno-psabi',
> +        ]
> +    endif
> +endif
> +
>  # We use C99 designated initializers for arrays as C++ has no equivalent
>  # feature. Both gcc and clang support this extension, but recent
>  # versions of clang generate a warning that needs to be disabled.
> @@ -71,16 +88,6 @@ if cc.has_argument('-Wno-c99-designator')
>      ]
>  endif
>  
> -# gcc 7.1 introduced processor-specific ABI breakages related to parameter
> -# passing on ARM platforms. This generates a large number of messages during
> -# compilation with gcc >=7.1 until gcc 9. Silence them.
> -if (host_machine.cpu_family() == 'arm' and cc.get_id() == 'gcc' and
> -    cc.version().version_compare('>=7.1') and cc.version().version_compare('<9'))
> -    cpp_arguments += [
> -        '-Wno-psabi',
> -    ]
> -endif
> -
>  c_arguments += common_arguments
>  cpp_arguments += common_arguments
>  
>

Hi Kieran,

On Mon, Aug 24, 2020 at 10:52:53AM +0100, Kieran Bingham wrote:
> On 22/08/2020 21:04, Laurent Pinchart wrote:
> > Due to popular request, move from C++14 to C++17. This will allow
> > dropping some custom constructs (such as a custom utils::clamp),
> > benefiting from new language features, and dropping gcc 5 and 6 from the
> > compilation tests.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Update documentation
> > - Print an error message when detecting a too old compiler
> 
> Ok, some useful additions... reading. ..
> 
> > ---
> >  Documentation/coding-style.rst |  5 ++---
> >  meson.build                    | 29 ++++++++++++++++++-----------
> >  2 files changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > index 7acba37b8de8..7c56a1b70014 100644
> > --- a/Documentation/coding-style.rst
> > +++ b/Documentation/coding-style.rst
> > @@ -88,13 +88,12 @@ headers, and with double quotes for other libcamera headers.
> >  C++ Specific Rules
> >  ------------------
> >  
> > -The code shall be implemented in C++14, with the following caveats:
> > +The code shall be implemented in C++17, with the following caveats:
> >  
> >  * Type inference (auto and decltype) shall be used with caution, to avoid
> >    drifting towards an untyped language.
> >  * The explicit, override and final specifiers are to be used where applicable.
> > -* General-purpose smart pointers (std::unique_ptr) deprecate std::auto_ptr.
> > -  Smart pointers, as well as shared pointers and weak pointers, shall not be
> > +* Smart pointers, as well as shared pointers and weak pointers, shall not be
> >    overused.
> >  * Classes are encouraged to define move constructors and assignment operators
> >    where applicable, and generally make use of the features offered by rvalue
> > diff --git a/meson.build b/meson.build
> > index ec54e68f3635..cf2636d97100 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -6,7 +6,7 @@ project('libcamera', 'c', 'cpp',
> >      default_options : [
> >          'werror=true',
> >          'warning_level=2',
> > -        'cpp_std=c++14',
> > +        'cpp_std=c++17',
> >      ],
> >      license : 'LGPL 2.1+')
> >  
> > @@ -62,6 +62,23 @@ if cc.get_id() == 'clang'
> >      endif
> >  endif
> >  
> > +if cc.get_id() == 'gcc'
> > +    if cc.version().version_compare('<7')
> > +        error('gcc version is too old, libcamera requires 7.0 or newer')
> > +    endif
> 
> Clang v5 or greater is required for C++17. Do we need to catch that too?
> or is that considered old enough that we're not going to hit it.

The lowest clang version installed on my system is 6.0, so I haven't
considered this. It's a good point, I'll add a check.

> I think it looks like Clang 5 was released around September 2017, while
> GCC 7.1 was released earlier in May 2017.
> 
> That said, wouldn't the compiler warn us quite verbosely if it didn't
> support C++17 ?
> 
> I.e. .. do /we/ really need to do this check here in meson?

Here's how the compiler warns me on gcc 5:

[4/281] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o'
FAILED: src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o 
g++-5.4.0 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o' -c ../../src/libcamera/event_notifier.cpp
In file included from ../../include/libcamera/object.h:14:0,
                 from ../../include/libcamera/event_notifier.h:10,
                 from ../../src/libcamera/event_notifier.cpp:8:
../../include/libcamera/bound_method.h:217:64: error: ‘maybe_unused’ attribute directive ignored [-Werror=attributes]
  R activate(Args... args, [[maybe_unused]] bool deleteMethod = false) override
                                                                ^
In file included from ../../include/libcamera/event_notifier.h:11:0,
                 from ../../src/libcamera/event_notifier.cpp:8:
../../include/libcamera/signal.h: In member function ‘void libcamera::Signal<Args>::disconnect()’:
../../include/libcamera/signal.h:73:66: error: ‘maybe_unused’ attribute directive ignored [-Werror=attributes]
   SignalBase::disconnect([]([[maybe_unused]] SlotList::iterator &iter) {
                                                                  ^
../../include/libcamera/signal.h: In instantiation of ‘libcamera::Signal<Args>::disconnect()::<lambda(std::__cxx11::list<libcamera::BoundMethodBase*>::iterator&)> [with Args = {libcamera::EventNotifier*}; std::__cxx11::list<libcamera::BoundMethodBase*>::iterator = std::_List_iterator<libcamera::BoundMethodBase*>]’:
../../include/libcamera/signal.h:73:27:   required from ‘struct libcamera::Signal<Args>::disconnect() [with Args = {libcamera::EventNotifier*}]::<lambda(std::__cxx11::list<libcamera::BoundMethodBase*>::iterator&)>’
../../include/libcamera/signal.h:73:25:   required from ‘void libcamera::Signal<Args>::disconnect() [with Args = {libcamera::EventNotifier*}]’
../../include/libcamera/signal.h:44:13:   required from ‘libcamera::Signal<Args>::~Signal() [with Args = {libcamera::EventNotifier*}]’
../../src/libcamera/event_notifier.cpp:67:56:   required from here
../../include/libcamera/signal.h:73:66: error: unused parameter ‘iter’ [-Werror=unused-parameter]
cc1plus: all warnings being treated as errors

I'd rather have

../../meson.build:66:8: ERROR: Problem encountered: gcc version is too old, libcamera requires 7.0 or newer

:-)

> Anyway,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Still stands.
> 
> > +
> > +    # gcc 7.1 introduced processor-specific ABI breakages related to parameter
> > +    # passing on ARM platforms. This generates a large number of messages
> > +    # during compilation with gcc >=7.1 until gcc 9. Silence them.
> > +    if (host_machine.cpu_family() == 'arm' and
> > +        cc.version().version_compare('>=7.1') and
> > +        cc.version().version_compare('<9'))
> > +        cpp_arguments += [
> > +            '-Wno-psabi',
> > +        ]
> > +    endif
> > +endif
> > +
> >  # We use C99 designated initializers for arrays as C++ has no equivalent
> >  # feature. Both gcc and clang support this extension, but recent
> >  # versions of clang generate a warning that needs to be disabled.
> > @@ -71,16 +88,6 @@ if cc.has_argument('-Wno-c99-designator')
> >      ]
> >  endif
> >  
> > -# gcc 7.1 introduced processor-specific ABI breakages related to parameter
> > -# passing on ARM platforms. This generates a large number of messages during
> > -# compilation with gcc >=7.1 until gcc 9. Silence them.
> > -if (host_machine.cpu_family() == 'arm' and cc.get_id() == 'gcc' and
> > -    cc.version().version_compare('>=7.1') and cc.version().version_compare('<9'))
> > -    cpp_arguments += [
> > -        '-Wno-psabi',
> > -    ]
> > -endif
> > -
> >  c_arguments += common_arguments
> >  cpp_arguments += common_arguments
> >

Hi Laurent,

Thanks for your work.

On 2020-08-24 20:57:24 +0300, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Aug 24, 2020 at 10:52:53AM +0100, Kieran Bingham wrote:
> > On 22/08/2020 21:04, Laurent Pinchart wrote:
> > > Due to popular request, move from C++14 to C++17. This will allow
> > > dropping some custom constructs (such as a custom utils::clamp),
> > > benefiting from new language features, and dropping gcc 5 and 6 from the
> > > compilation tests.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Update documentation
> > > - Print an error message when detecting a too old compiler
> > 
> > Ok, some useful additions... reading. ..
> > 
> > > ---
> > >  Documentation/coding-style.rst |  5 ++---
> > >  meson.build                    | 29 ++++++++++++++++++-----------
> > >  2 files changed, 20 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> > > index 7acba37b8de8..7c56a1b70014 100644
> > > --- a/Documentation/coding-style.rst
> > > +++ b/Documentation/coding-style.rst
> > > @@ -88,13 +88,12 @@ headers, and with double quotes for other libcamera headers.
> > >  C++ Specific Rules
> > >  ------------------
> > >  
> > > -The code shall be implemented in C++14, with the following caveats:
> > > +The code shall be implemented in C++17, with the following caveats:
> > >  
> > >  * Type inference (auto and decltype) shall be used with caution, to avoid
> > >    drifting towards an untyped language.
> > >  * The explicit, override and final specifiers are to be used where applicable.
> > > -* General-purpose smart pointers (std::unique_ptr) deprecate std::auto_ptr.
> > > -  Smart pointers, as well as shared pointers and weak pointers, shall not be
> > > +* Smart pointers, as well as shared pointers and weak pointers, shall not be
> > >    overused.
> > >  * Classes are encouraged to define move constructors and assignment operators
> > >    where applicable, and generally make use of the features offered by rvalue
> > > diff --git a/meson.build b/meson.build
> > > index ec54e68f3635..cf2636d97100 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -6,7 +6,7 @@ project('libcamera', 'c', 'cpp',
> > >      default_options : [
> > >          'werror=true',
> > >          'warning_level=2',
> > > -        'cpp_std=c++14',
> > > +        'cpp_std=c++17',
> > >      ],
> > >      license : 'LGPL 2.1+')
> > >  
> > > @@ -62,6 +62,23 @@ if cc.get_id() == 'clang'
> > >      endif
> > >  endif
> > >  
> > > +if cc.get_id() == 'gcc'
> > > +    if cc.version().version_compare('<7')
> > > +        error('gcc version is too old, libcamera requires 7.0 or newer')
> > > +    endif
> > 
> > Clang v5 or greater is required for C++17. Do we need to catch that too?
> > or is that considered old enough that we're not going to hit it.
> 
> The lowest clang version installed on my system is 6.0, so I haven't
> considered this. It's a good point, I'll add a check.

With this check added,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> 
> > I think it looks like Clang 5 was released around September 2017, while
> > GCC 7.1 was released earlier in May 2017.
> > 
> > That said, wouldn't the compiler warn us quite verbosely if it didn't
> > support C++17 ?
> > 
> > I.e. .. do /we/ really need to do this check here in meson?
> 
> Here's how the compiler warns me on gcc 5:
> 
> [4/281] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o'
> FAILED: src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o 
> g++-5.4.0 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/event_notifier.cpp.o' -c ../../src/libcamera/event_notifier.cpp
> In file included from ../../include/libcamera/object.h:14:0,
>                  from ../../include/libcamera/event_notifier.h:10,
>                  from ../../src/libcamera/event_notifier.cpp:8:
> ../../include/libcamera/bound_method.h:217:64: error: ‘maybe_unused’ attribute directive ignored [-Werror=attributes]
>   R activate(Args... args, [[maybe_unused]] bool deleteMethod = false) override
>                                                                 ^
> In file included from ../../include/libcamera/event_notifier.h:11:0,
>                  from ../../src/libcamera/event_notifier.cpp:8:
> ../../include/libcamera/signal.h: In member function ‘void libcamera::Signal<Args>::disconnect()’:
> ../../include/libcamera/signal.h:73:66: error: ‘maybe_unused’ attribute directive ignored [-Werror=attributes]
>    SignalBase::disconnect([]([[maybe_unused]] SlotList::iterator &iter) {
>                                                                   ^
> ../../include/libcamera/signal.h: In instantiation of ‘libcamera::Signal<Args>::disconnect()::<lambda(std::__cxx11::list<libcamera::BoundMethodBase*>::iterator&)> [with Args = {libcamera::EventNotifier*}; std::__cxx11::list<libcamera::BoundMethodBase*>::iterator = std::_List_iterator<libcamera::BoundMethodBase*>]’:
> ../../include/libcamera/signal.h:73:27:   required from ‘struct libcamera::Signal<Args>::disconnect() [with Args = {libcamera::EventNotifier*}]::<lambda(std::__cxx11::list<libcamera::BoundMethodBase*>::iterator&)>’
> ../../include/libcamera/signal.h:73:25:   required from ‘void libcamera::Signal<Args>::disconnect() [with Args = {libcamera::EventNotifier*}]’
> ../../include/libcamera/signal.h:44:13:   required from ‘libcamera::Signal<Args>::~Signal() [with Args = {libcamera::EventNotifier*}]’
> ../../src/libcamera/event_notifier.cpp:67:56:   required from here
> ../../include/libcamera/signal.h:73:66: error: unused parameter ‘iter’ [-Werror=unused-parameter]
> cc1plus: all warnings being treated as errors
> 
> I'd rather have
> 
> ../../meson.build:66:8: ERROR: Problem encountered: gcc version is too old, libcamera requires 7.0 or newer
> 
> :-)
> 
> > Anyway,
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Still stands.
> > 
> > > +
> > > +    # gcc 7.1 introduced processor-specific ABI breakages related to parameter
> > > +    # passing on ARM platforms. This generates a large number of messages
> > > +    # during compilation with gcc >=7.1 until gcc 9. Silence them.
> > > +    if (host_machine.cpu_family() == 'arm' and
> > > +        cc.version().version_compare('>=7.1') and
> > > +        cc.version().version_compare('<9'))
> > > +        cpp_arguments += [
> > > +            '-Wno-psabi',
> > > +        ]
> > > +    endif
> > > +endif
> > > +
> > >  # We use C99 designated initializers for arrays as C++ has no equivalent
> > >  # feature. Both gcc and clang support this extension, but recent
> > >  # versions of clang generate a warning that needs to be disabled.
> > > @@ -71,16 +88,6 @@ if cc.has_argument('-Wno-c99-designator')
> > >      ]
> > >  endif
> > >  
> > > -# gcc 7.1 introduced processor-specific ABI breakages related to parameter
> > > -# passing on ARM platforms. This generates a large number of messages during
> > > -# compilation with gcc >=7.1 until gcc 9. Silence them.
> > > -if (host_machine.cpu_family() == 'arm' and cc.get_id() == 'gcc' and
> > > -    cc.version().version_compare('>=7.1') and cc.version().version_compare('<9'))
> > > -    cpp_arguments += [
> > > -        '-Wno-psabi',
> > > -    ]
> > > -endif
> > > -
> > >  c_arguments += common_arguments
> > >  cpp_arguments += common_arguments
> > >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel