[vs2017] warning C4265: class has virtual functions, but destructor is not virtual

[matlo607]

VS2017 triggers the following warnings:

cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\step\stepmanager.hpp(114): warning C4265: 'cucumber::internal::StepInfo': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\step\stepmanager.hpp(163): warning C4265: 'cucumber::internal::BasicStep': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(38): warning C4265: 'cucumber::internal::Hook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(40): warning C4265: 'cucumber::internal::BeforeHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(48): warning C4265: 'cucumber::internal::AroundStepHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(50): warning C4265: 'cucumber::internal::AfterStepHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(52): warning C4265: 'cucumber::internal::AfterHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(57): warning C4265: 'cucumber::internal::UnconditionalHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(59): warning C4265: 'cucumber::internal::BeforeAllHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\hook\hookregistrar.hpp(61): warning C4265: 'cucumber::internal::AfterAllHook': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\drivers\gtestdriver.hpp(22): warning C4265: 'cucumber::internal::GTestStep': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager0': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager1': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager2': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager3': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager4': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager5': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager6': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager7': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
warning C4265: 'CukeObjectDeliveryManager8': class has virtual functions, but destructor is not virtual
         instances of this class may not be destructed correctly
cucumber-cpp\6390bee9b32\include\cucumber-cpp\internal\step\stepmanager.hpp(172): warning C4265: 'cucumber::internal::StepInvoker<T>': class has virtual functions, but destructor is not virtual

[coveralls]

Coverage decreased (-0.4%) to 62.456% when pulling 7888fd8 on matlo607:vs2017-fix-warnings into 79e92de on cucumber:master.

[konserw]

Hi, could you please squash it to one commit? Other than that it looks good to me :)
Thanks for contribution!

[muggenhor]

This thing is not a problem because we never delete through a pointer to the base class. So this warning is in effect a false positive.

We already have a few Visual Studio-only warning workarounds in that add code with no other value than that. Is there another way to make it stop giving this warning? If not, then I want to see a comment near every virtual destructor that you add that the sole reason is this particular warning (C4265) of visual studio.

[matlo607]

@muggenhor, I found this discussion: [StackOverflow]Should every class have a virtual destructor?
Some people share your opinion, others prefer more explicit code.
Since this class will never be deleted through a pointer to the base class, you are right, it is not necessary to specify it.
At the same time, is the space used by a vtable that important ?
In my opinion, making the code more explicit is not a bad thing and is preferable to surrounding the code with:

#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4265)
#endif

// ...

#ifdef _MSC_VER
#pragma warning(pop)
#endif

Both points of view are defendable. Please tell me what you decide and I will change the pull request accordingly?

[muggenhor]

Since this class will never be deleted through a pointer to the base class, you are right, it is not necessary to specify it.
At the same time, is the space used by a vtable that important?

Given that these classes already have virtual functions they already have a vtable. So while the size of that will increase, the size of the pointer to the vtable won't increase (that's the only per-object item). So that's not the issue here.

The problem is that you're violating the rule of 0 (specify no special member functions), without following the rule of 3/rule of 5 (specify all special member functions). That's the real cost of specifying a virtual destructor without needing it: you incur a larger maintenance cost. That's not a problem if there's a need for it, there isn't.

In my opinion, making the code more explicit is not a bad thing
Explicitly doing stuff (overriding the default implementations of special members) is in my experience not a good thing as it makes the code more difficult to read (by having more to read). If you want to be able to use the language you need to understand the rules for the defaults anyway, so making things explicit in some locations doesn't save you from that need.

and is preferable to surrounding the code with ... snip ... Both points of view are defendable. Please tell me what you decide and I will change the pull request accordingly?

I agree that the pragmas for disabling these warnings are not desired. Instead I prefer to have these disabled with target_compile_options(${TARGET} PUBLIC /wd4265) inside src/CMakeLists.txt combined with a comment that this is a false positive.

[matlo607]

Actually I got these errors in client code...
target_compile_options(${TARGET} PUBLIC /wd4265) is not a suitable option in this case.

[muggenhor]

Right, well that's disappointing.

[matlo607]

We already have a few Visual Studio-only warning workarounds in that add code with no other value than that. Is there another way to make it stop giving this warning? If not, then I want to see a comment near every virtual destructor that you add that the sole reason is this particular warning (C4265) of visual studio.

@muggenhor, please could you review the last version ?

[matlo607]

Hello @muggenhor,
Please could you review the PR after my changes ?

[gms1]

this thing IS a problem:

gms@orion:~$ build-cucumber-cpp 2>&1 | grep error
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInfoNoOp' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInfoPending' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInfoWithTableArg' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInvoker<CukeObject0>' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInvoker<CukeObject1>' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInvoker<CukeObject2>' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInvoker<CukeObject3>' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
7 errors generated.
gms@orion:~$ 

detail:

[ 64%] Building CXX object tests/CMakeFiles/GenericDriverTest.dir/integration/drivers/GenericDriverTest.cpp.o
In file included from /home/gms/projbuild/cucumber-cpp/tests/integration/drivers/GenericDriverTest.cpp:1:
In file included from /home/gms/projbuild/cucumber-cpp/include/cucumber-cpp/generic.hpp:1:
In file included from /home/gms/projbuild/cucumber-cpp/include/cucumber-cpp/internal/defs.hpp:1:
In file included from /home/gms/projbuild/cucumber-cpp/include/cucumber-cpp/internal/drivers/../step/StepManager.hpp:12:
In file included from /usr/include/boost/make_shared.hpp:14:
In file included from /usr/include/boost/smart_ptr/make_shared.hpp:14:
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:13: error: destructor called on non-final 'cucumber::internal::StepInfoNoOp' that has virtual functions but non-virtual destru
ctor [-Werror,-Wdelete-non-virtual-dtor]
            p->~T();
            ^
/usr/include/boost/smart_ptr/make_shared_object.hpp:88:9: note: in instantiation of member function 'boost::detail::sp_ms_deleter<cucumber::internal::StepInfoNoOp>::destroy' request
ed here
        destroy();
        ^
/usr/include/boost/smart_ptr/detail/sp_counted_impl.hpp:166:5: note: in instantiation of member function 'boost::detail::sp_ms_deleter<cucumber::internal::StepInfoNoOp>::~sp_ms_dele
ter' requested here
    sp_counted_impl_pd( P p ): ptr( p ), del()
    ^
/usr/include/boost/smart_ptr/detail/shared_count.hpp:213:23: note: in instantiation of member function 'boost::detail::sp_counted_impl_pd<cucumber::internal::StepInfoNoOp *, boost::
detail::sp_ms_deleter<cucumber::internal::StepInfoNoOp> >::sp_counted_impl_pd' requested here
            pi_ = new sp_counted_impl_pd< P, D >( p );
                      ^
/usr/include/boost/smart_ptr/shared_ptr.hpp:388:67: note: in instantiation of function template specialization 'boost::detail::shared_count::shared_count<cucumber::internal::StepInf
oNoOp *, boost::detail::sp_ms_deleter<cucumber::internal::StepInfoNoOp> >' requested here
    template<class Y, class D> shared_ptr( Y * p, D d ): px( p ), pn( p, d )
                                                                  ^
/usr/include/boost/smart_ptr/make_shared_object.hpp:250:28: note: in instantiation of function template specialization 'boost::shared_ptr<cucumber::internal::StepInfoNoOp>::shared_p
tr<cucumber::internal::StepInfoNoOp, boost::detail::sp_inplace_tag<boost::detail::sp_ms_deleter<cucumber::internal::StepInfoNoOp> > >' requested here
    boost::shared_ptr< T > pt( static_cast< T* >( 0 ), BOOST_SP_MSD( T ) );
                           ^
/home/gms/projbuild/cucumber-cpp/tests/integration/drivers/../../utils/StepManagerTestDouble.hpp:64:31: note: in instantiation of function template specialization 
'boost::make_shared<cucumber::internal::StepInfoNoOp, const std::__cxx11::basic_string<char> &, char const (&)[1]>' requested here
        return addStep(boost::make_shared<StepInfoNoOp>(stepMatcher, ""));
                              ^
/usr/include/boost/smart_ptr/make_shared_object.hpp:59:17: note: qualify call to silence this warning
            p->~T();
                ^
/

[ghost]

The fact that this issue comes up not only with test harness but also with examples makes me think that derivatives of class Hook and class StepInfo are deleted by pointer. If that wasn't the case then making destructors protected would've been enough as it is with class BasicStep.

@muggenhor @matlo607 I don't have write access but I've checked this locally with clang and it seem to work just as fine so I just went ahead and started a new pull request #209

[ghost]

@muggenhor

This thing is not a problem because we never delete through a pointer to the base class. So this warning is in effect a false positive.

Not true:

cucumber-cpp/tests/utils/StepManagerTestDouble.hpp

Line 73 in 9ac02a4

boost::shared_ptr<StepInfo> stepInfo(boost::make_shared<StepInfoNoOp>(stepMatcher, source));

cucumber-cpp/tests/utils/StepManagerTestDouble.hpp

Line 84 in 9ac02a4

boost::shared_ptr<StepInfo> stepInfo(boost::make_shared<StepInfoPending>(stepMatcher, description));

cucumber-cpp/tests/utils/StepManagerTestDouble.hpp

Line 90 in 9ac02a4

boost::shared_ptr<StepInfo> stepInfo(boost::make_shared<StepInfoWithTableArg>(stepMatcher, expectedSize));

[paoloambrosio]

@matlo607 Thanks for your contribution and apologies for looking at this quite late.

To be honest, I would remove the comments. This PR in the repo history should be enough documentation and I'd rather not have clutter.

May I ask you to remove them? I'll merge it right away afterwards.

[matlo607]

@paoloambrosio I removed the comments.

[paoloambrosio]

No idea what is happening with the Appveyor build 🤦‍♂️

See discussion in this same PR

[konserw]

Switching over to ruby 2.2.x on appveyor fixes problem seen here, so at least it builds and unit tests are running ok, but then e2e tests are failing, see:
https://ci.appveyor.com/project/konserw/cucumber-cpp/build/job/4ackex31k84oa1qg
Cucumber 2.0.0 is too old for ruby 2.2, or at least gherkin < 3, see here:
cucumber-attic/gherkin#259

It's our own dependency hell as bug in cucumber > 2.0 won't work with cucumber-cpp :/

[paoloambrosio]

Created #217 to fix the build.

[paoloambrosio]

Verified this builds when rebased against current master. Merging it even if the build didn't pass at the time.