final-keyword

[jasonimercer]

Add final keyword to child class since parent has virtual methods and grand parent doesn't have a virtual destructor (it's based on a .msg file). This allows the code to be compiled without warnings by pedantic compilers like clang 6.0 and above.

Has no impact on pre-C++11 compilers.

A minimal program to help understand this issue is as follows:

#include <memory>

struct Base
{
  using Ptr = std::shared_ptr<Base>;
  virtual void f() = 0;
};


struct Child /*final*/ : Base
{
  void f()
  {
  }
};

int main()
{
  Child::Ptr c = std::make_shared<Child>();
  c->f();
}

compiling this with clang-6.0 gives:

jmercer@cpr00549:~/.../scratch$ clang++-6.0 --std=c++11 -Wall main.cpp 
In file included from main.cpp:1:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/memory:63:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/allocator.h:46:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/x86_64-linux-gnu/c++/5.4.0/bits/c++allocator.h:33:
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/ext/new_allocator.h:124:29: warning: destructor called on
      non-final 'Child' that has virtual functions but non-virtual destructor [-Wdelete-non-virtual-dtor]
        destroy(_Up* __p) { __p->~_Up(); }
                            ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/alloc_traits.h:542:8: note: in instantiation of function
      template specialization '__gnu_cxx::new_allocator<Child>::destroy<Child>' requested here
        { __a.destroy(__p); }
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr_base.h:531:28: note: in instantiation of
      function template specialization 'std::allocator_traits<std::allocator<Child> >::destroy<Child>' requested here
        allocator_traits<_Alloc>::destroy(_M_impl._M_alloc(), _M_ptr());
                                  ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr_base.h:517:2: note: in instantiation of member
      function 'std::_Sp_counted_ptr_inplace<Child, std::allocator<Child>, __gnu_cxx::_S_atomic>::_M_dispose' requested here
        _Sp_counted_ptr_inplace(_Alloc __a, _Args&&... __args)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr_base.h:617:18: note: in instantiation of
      function template specialization 'std::_Sp_counted_ptr_inplace<Child, std::allocator<Child>,
      __gnu_cxx::_S_atomic>::_Sp_counted_ptr_inplace<>' requested here
          ::new (__mem) _Sp_cp_type(std::move(__a),
                        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr_base.h:1096:14: note: in instantiation of
      function template specialization 'std::__shared_count<__gnu_cxx::_S_atomic>::__shared_count<Child, std::allocator<Child>>'
      requested here
        : _M_ptr(), _M_refcount(__tag, (_Tp*)0, __a,
                    ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr.h:319:4: note: in instantiation of function
      template specialization 'std::__shared_ptr<Child, __gnu_cxx::_S_atomic>::__shared_ptr<std::allocator<Child>>' requested here
        : __shared_ptr<_Tp>(__tag, __a, std::forward<_Args>(__args)...)
          ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr.h:619:14: note: in instantiation of function
      template specialization 'std::shared_ptr<Child>::shared_ptr<std::allocator<Child>>' requested here
      return shared_ptr<_Tp>(_Sp_make_shared_tag(), __a,
             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/shared_ptr.h:635:19: note: in instantiation of function
      template specialization 'std::allocate_shared<Child, std::allocator<Child>>' requested here
      return std::allocate_shared<_Tp>(std::allocator<_Tp_nc>(),
                  ^
main.cpp:19:23: note: in instantiation of function template specialization 'std::make_shared<Child>' requested here
  Child::Ptr c = std::make_shared<Child>();
                      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/ext/new_allocator.h:124:35: note: qualify call to silence this
      warning
        destroy(_Up* __p) { __p->~_Up(); }
                                  ^
1 warning generated.
jmercer@cpr00549:~/.../scratch$ 

[jasonimercer]

Looks like some tests are timing out. Could someone re-kick them or verify the test environment is correct? These tests are also failing in other PRs.

[mikaelarguedas]

thanks @jasonimercer for the improvement!

Test failure is unrelated, retriggering CI now that the test failure has been fixed on master
@ros-pull-request-builder retest this please

Just to confirm, this change does change ABI isn't it ?

[jasonimercer]

This would not result in an ABI change. If someone is creating a dynamic reconfigure message and then they are trying to inherit from it and they are on C+11 or better then this will result in a compilation error. This use-case is pretty specific though and I struggle to imagine what the intention would be. Generally if you want some custom data in a dynamic reconfigure you'll end up writing a new one rather than trying to re-use some fields.

If the user wanted to re-use some fields then the language offers options such as templated functions.

I've been using this branch at Clearpath for about 1/2 a year now without issue. It's nice to be able to crank up warnings an not get spammed by issues from upstream.

I'd be happy to add to this commit and add some comments around the final keyword and where it's used in the source. If someone ever encounters an issue with it we can help them understand where they may have gone astray and offer some options inline.

[mikaelarguedas]

This use-case is pretty specific though and I struggle to imagine what the intention would be.

Yeah I agree that I haven't seen that use case around.

It's nice to be able to crank up warnings an not get spammed by issues from upstream.

👍

I'd be happy to add to this commit and add some comments around the final keyword and where it's used in the source.

That would be great for posterity.

The code change looks good to me.
It looks like now all platforms even windows report the correct value for __cplusplus so no need to add platform specific logic around it.

[jasonimercer]

The checks for __cplusplus are there so that we don't break this package for people stuck using a compiler before C++11 (final keyword added in 11). If we are happy saying dynamic_reconfigure is only C++11 or better then I can remove the checks.

I'll add a commit to the PR now with the inline comments on why there's a final keyword.

[mikaelarguedas]

If we are happy saying dynamic_reconfigure is only C++11 or better then I can remove the checks.

As currently this branch is being used for all ROS distros we will need to keep the checks for now.

I'll add a commit to the PR now with the inline comments on why there's a final keyword.

e2a2b34 lgtm, thanks!