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

[Laurent Pinchart]

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 at 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 at sha/event_notifier.cpp.o'
FAILED: src/libcamera/4ab8042@@camera at sha/event_notifier.cpp.o 
g++-5.4.0 -Isrc/libcamera/4ab8042@@camera at 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 at sha/event_notifier.cpp.o' -MF 'src/libcamera/4ab8042@@camera at sha/event_notifier.cpp.o.d' -o 'src/libcamera/4ab8042@@camera at 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 at 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