Am 14.10.19 um 21:28 schrieb Emil Dotchevski via Boost:

On Mon, Oct 14, 2019 at 9:36 AM Paul A Bristow via Boost < boost@lists.boost.org> wrote:

...

Using Clang 9 on Windows b2 toolset-clang-linux cxxstd=2a .

I am seeing lots of these warnings

In file included from ..\..\..\boost/test/included/unit_test.hpp:23: In file included from ..\..\..\boost/test/impl/execution_monitor.ipp:37: In file included from ..\..\..\boost/exception/diagnostic_information.hpp:11: ..\..\..\boost/exception/info.hpp:134:21: warning: delete called on non-final 'boost::exception_detail::error_info_container_impl' that has virtual functions but non-virtual destructor

[-Wdelete-non-abstract-non-virtual-dtor] delete this; ^ It seems that trying to supress is ineffective with

-Wno-delete-non-abstract-non-virtual-dtor

Perhaps by design?

https://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html#w... te-non-abstract-non-virtual-dtor https://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html#w... -Wdelete-non-abstract-non-virtual-dtor < https://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html>

Diagnostic text: warning: delete

destructor

called on non-final B that has virtual functions but non-virtual destructor -Wdelete-non-virtual-dtor < https://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html>

Some of the diagnostics controlled by this flag are enabled by default. Controls -Wdelete-abstract-non-virtual-dtor < https://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html> , -Wdelete-non-abstract-non-virtual-dtor < https://releases.llvm.org/9.0.0/tools/clang/docs/DiagnosticsReference.html> . Should I be concerned about this?

You shouldn't be concerned about this and also you shouldn't be getting warnings. What platforms, what command line?

This seems to be the same issue as https://github.com/boostorg/exception/issues/23 |boost_warnings_minimal_demo\boost_1_64_0\boost/exception/exception.hpp(176): error C4265: 'boost::exception_detail::error_info_container': class has virtual functions, but destructor is not virtual | Where does 'exception.hpp' come from? I'd expect it at https://github.com/boostorg/exception/tree/develop/include/boost/exception/e... but it isn't there...

Am 15.10.19 um 10:48 schrieb Andrey Semashev via Boost:

On 2019-10-15 10:06, Alexander Grund via Boost wrote:

...

Where does 'exception.hpp' come from? I'd expect it at https://github.com/boostorg/exception/tree/develop/include/boost/exception/e... but it isn't there...

It's in https://github.com/boostorg/throw_exception.

Then it looks like the warning is indeed correct: https://github.com/boostorg/throw_exception/blob/e2e802e5085e10db7d5ee5ded4c... `error_info_container` has a non-virtual dtor although it does have other virtual methods and the derived class has a `delete this` but is not final. Hence the compiler can't be sure that there isn't another derived class. Any reason to NOT make the dtor virtual? Performance degrade can be resolved by making the derived class final (where supported which should be almost everywhere nowadays)

On 2019-10-15 12:06, Alexander Grund via Boost wrote:

...

Then it looks like the warning is indeed correct: https://github.com/boostorg/throw_exception/blob/e2e802e5085e10db7d5ee5ded4c...

`error_info_container` has a non-virtual dtor although it does have other virtual methods and the derived class has a `delete this` but is not final. Hence the compiler can't be sure that there isn't another derived class.

Any reason to NOT make the dtor virtual? Performance degrade can be resolved by making the derived class final (where supported which should be almost everywhere nowadays)

The warning is bogus because the object is never destroyed through the base class. Marking the destructor virtual would suggest that that is possible. And add a virtual call where none is needed. The warning is actually correct as one *could* have a class derived from`error_info_container_impl` which would then be deleted through a

Marking error_info_container_impl final should help, but we currently don't have a macro for that. This would make the code correct (to tell that it is correct). Shame

Am 15.10.19 um 11:14 schrieb Andrey Semashev via Boost: pointer to `error_info_container_impl` only. The destructor of `error_info_container` is protected, which is good. However the one of `error_info_container_impl` is not. It should probably be even private (if all deletes happen through its `release()` function) So in the current form one (the compiler and a human) cannot judge if there are derived objects and how they are destroyed. Of course you are right that this warning is a false-positive in this case, but it's hard/impossible to tell. That's why I suggested a virtual dtor, which avoids the problem, and a final class, which "devirtualizes" the call again. that there is no macro yet, so thanks for the PR.

Am 15.10.19 um 22:45 schrieb Emil Dotchevski via Boost:

The warning is not correct, and making the destructor virtual is wrong, because by design it is a bug to delete the base pointer type. I just disabled the warning for MSVC.

The warning is "Virtual class has non-virtual destructor so deleting it through a base class will lead to bugs", which is correct, isn't it? As mentioned: Neither the compiler nor a reviewer can confirm that a delete only ever happens through the most derived pointer because while error_info_container has a protected destructor (and hence cannot be deleted through a pointer to that class from outside), the error_info_container_impl has a public destructor and deletes itself through a this pointer. So if at any point in time someone derives from that class (which the missing final allows and there is no comment advising against that) you will have a bug. Don't get me wrong: I agree that it is not a bug here and everything is working as intended *currently*. I'm just saying that the warning is valid, and the Clang/GCC(?) warning is even more helpful in suggesting to make this class error_info_container_impl which it should be according to the design. While writing this: It seems MSVC already warns for the base class -.- Again this is a valid warning, but can be ignored for this specific class because the dtor is protected. I'm not a friend of globally disabling a warning (for the whole file) without an explanation. I'd expected it around error_info_container with a comment like `//Never deleted through base class pointer` as a note to future devs.

On Wed, Oct 16, 2019 at 2:51 AM Paul A Bristow via Boost < boost@lists.boost.org> wrote:

Since the Standard seems to suggest to me, (ill-informed on this language lawyers delight) that using keyword final where implemented, that that is the Right Thing To Do long-term.

So I favour implementing BOOST_HAS_FINAL and using it in the code.

The problem is that this is not a solution for old code, since it may be used with old compiler versions, which requires the warning to be silenced anyway. Once the warning is silenced, adding final would not be needed. Can't hurt though.

Am 16.10.19 um 19:38 schrieb Emil Dotchevski via Boost:

The destructor is not virtual by design, not by mistake. The code is correct as written, so the warning is to be silenced.

I think we already agreed that the warning is a false positive in this case. Only thing I'd complain about the current change: It silences the warning for the whole file, not only the class.

...

So I favour implementing BOOST_HAS_FINAL and using it in the code.

The problem is that this is not a solution for old code, since it may be used with old compiler versions, which requires the warning to be silenced anyway. Once the warning is silenced, adding final would not be needed. Can't hurt though.

Yes and no. The base class is "designed to not be deleted from outside" by making the dtor protected. All good here. The derived class has a public dtor and is not final (and has no comment that one should not derive from it). This opens room for the bug the compiler (GCC/Clang) warns you about. Solution a) Make the class final (the upcoming BOOST_FINAL also serves as a "comment" to programmers even though it has no effect on older compilers) Solution b) Additionally make the destructor private (iff `release()` is the only valid way to delete the class) Note that this is needed to avoid *future* bugs by documenting (in code) the expected usage in a way the programmer AND (newer) compilers do understand. My argument for making the base class destructor virtual was this: - Code is safe by default even after refactoring (e.g. for whatever reason:`void release(){ ...; error_container* c = this; ...; delete c; }` ) - Adding `final` will remove the virtual destructor call in current code,n making the code fast - No warning suppression is needed - Counter argument: On compilers not supporting "final" this will be a little bit slower on destruction