Community
Participate
Working Groups
class A { public: A(); virtual ~A(); virtual void m(); }; class B : public A { public: ~B(); //Warning: Class 'B' has virtual method 'm' but non-virtual destructor '~B' void m(); }; The warning is wrong since ~B is virtual even without the virtual keyword.
It is virtual because ~A is virtual?
(In reply to comment #1) > It is virtual because ~A is virtual? Yes.
Shouldn't the only error be: hasOwnVirtualMethod && hasOwnNonVirDestructor && !HasVirtualDestructorInBases(classType) ? If so, I can attach a patch.
I get a lot of warnings from CODAN about using a non-virtual destructor altough the gcc compiler does not complain about it (you can activate the gcc warning with the option -Wnon-virtual-dtor). As suggested in comment #2 the warning is invalid because ~B is already virtual. BTW, there is another bug when the destructor is declared protected. e.g. class A { public: A(); virtual void m() = 0; protected: ~A(); }; In this case the destructor can never be called. So no memory leak can actually happen. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15214 and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=7302
Marc or Sergey can you fix it? I have no idea what it suppose to do and why it is even and error and I don't have a code base to test my changes.
(In reply to comment #5) I don't have time now, but can look at it in early April.
Hi, I am trying CDT 8.0 M7, and getting lots of false warnings regarding non-virtual destructor. Are there any plans of fixing this issue for CDT 8.0 time frame? Thanks a lot, Yevgeny
I'm working on this. Almost finished.
Created attachment 195938 [details] fix This should do the trick. Feedback is welcome.
Created attachment 196020 [details] fix2 I didn't read comment4 careful enough. This fixes one issue more. Still some work to do. New testcase from the provided links is the following code: struct A { virtual void f() = 0; protected: ~A(); // ok. }; struct B { virtual void f() = 0; ~B(); // warn! public non-virtual dtor. }; struct C { virtual void f() = 0; // warn! implicit public non-virtual dtor. }; struct D { virtual void f() = 0; private: friend class C; ~D(); // warn! can be called from class C. };
(In reply to comment #10) I've committed the patch with some minor modifications. The most important of them being that this checker should not pay any attention to destructor visibility. Visibility checks belong to a separate checker and should not be limited to destructors. Please add tests to AbstractClassInstantiationCheckerTest.java.
(In reply to comment #11) Thanks for taking care of this one! And for improving my patch. > ... > The most important of > them being that this checker should not pay any attention to destructor > visibility. .... This visibility check was meant to solve the false positive mentioned in comment #4: class A { public: A(); virtual void m() = 0; protected: ~A(); // do _not_ warn here... }; I still think this check belongs to this checker in order to solve this. > Visibility checks belong to a separate checker and should not be limited to destructors. Since checkers can be enabled individually we should handle this in a self contained way. Imagine users having enabled just this one here. > Please add tests to AbstractClassInstantiationCheckerTest.java. Will do. As soon as time permits me to do that. (That's not on my day-job) I intend to add to add those additional test cases mentioned in comment #10. This will result in the test failing on C() and D(). Is that a problem?
*** cdt cvs genie on behalf of sprigogin *** Bug 315528 - Non-virtual destructor diagnostics doesn't take superclass into account. Patch by Patrick Hofer. [*] NonVirtualDestructor.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java?root=Tools_Project&r1=1.5&r2=1.6
(In reply to comment #12) > > class A { > public: > A(); > virtual void m() = 0; > protected: > ~A(); // do _not_ warn here... > }; I don't understand why there should be no warning in this case. Consider the following example: class A { public: A(); virtual void m() = 0; protected: ~A(); }; class B : public A { void foo() { A* a = new B(); delete a; // ~B() is not being called. } };
(In reply to comment #14) > I don't understand why there should be no warning in this case. Consider the > following example: > > class A { > public: > A(); > virtual void m() = 0; > protected: > ~A(); > }; > > class B : public A { > void foo() { > A* a = new B(); > delete a; // ~B() is not being called. > } > }; Your code won't compile (at least not with gcc). ../src/hello.cpp: In member function ‘void B::foo()’: ../src/hello.cpp:31: error: ‘A::~A()’ is protected ../src/hello.cpp:37: error: within this context Making the destructor protected prevents the class to be subclassed.
(In reply to comment #15) Thanks, Axel. I've added back the visibility condition with an additional check for absence of friends.
*** cdt cvs genie on behalf of sprigogin *** Bug 315528 - Non-virtual destructor diagnostics doesn't take superclass into account. Fixed false negatives when dtor is not public and there are no friends. [*] NonVirtualDestructor.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java?root=Tools_Project&r1=1.6&r2=1.7
(In reply to comment #16) > (In reply to comment #15) >.... I've added back the visibility condition with an additional check > for absence of friends. Thanks, Sergey! I have my unit tests almost finished. Will attach a patch tomorrow. By creating those tests, I have discovered a few more issues and have fixed them. And then I found other false positives while running the latest nightly, with your commit in (the one from comment #23), on our production code today. (Btw, we just fixed the friend case independently, but that doesn't hurt ;-) )
(In reply to comment #18) > ... Will attach a patch tomorrow. Just noticed lots of incoming changes. This has to wait for next week.
Created attachment 196318 [details] testcases I have created a new unit test file. Those many cases to consider are involved enough to warrant its own test set. (I've already added 'Checker' to the Name, because I think we should rename the checker class, but let's postpone this until git is in place...)
Created attachment 196319 [details] more fixes and some typos resolved This fixes some more test cases. There are some TODO markers left in the code. Is there a way to have different error strings, without populating more checkers to the UI? I couldn't figure out how to do that in. I'm going to work further on this.
(In reply to comment #21) > There are some TODO markers left in the code. Is there a way to have different > error strings, without populating more checkers to the UI? I couldn't figure > out how to do that in. The way to do that in Codan is to have multiple problem types associated with a single checker. "implicit public ~" + clazz + "() generated by the compiler" is too verbose. Every C++ programmer has to know that default ctors and dtors may be generated by the compiler.
*** cdt cvs genie on behalf of sprigogin *** Bug 315528 - Non-virtual destructor diagnostics doesn't take superclass into account. Tests by Patrick Hofer. [*] NonVirtualDestructor.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java?root=Tools_Project&r1=1.10&r2=1.11 [*] AutomatedIntegrationSuite.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/test/AutomatedIntegrationSuite.java?root=Tools_Project&r1=1.20&r2=1.21 [+] NonVirtualDestructorCheckerTest.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java?root=Tools_Project&revision=1.1&view=markup
Created attachment 196506 [details] fix4 Reduced verbosity of message as suggested. I still regard having just one single message as a limitation within codan. Having slightly different ones without cluttering the UI might be useful. This patch fixes all the test cases we have so far.
Patch applied. testNonVirtualDtorInBaseClass2 is still failing, although I don't agree with the test that the warning should be reported not just on A but on the derived classes too.
*** cdt cvs genie on behalf of sprigogin *** Bug 315528 - Non-virtual destructor diagnostics doesn't take superclass into account. Patch by Patrick Hofer. [*] NonVirtualDestructorCheckerTest.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java?root=Tools_Project&r1=1.1&r2=1.2 [*] plugin.xml 1.38 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/plugin.xml?root=Tools_Project&r1=1.37&r2=1.38 [*] NonVirtualDestructor.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java?root=Tools_Project&r1=1.11&r2=1.12 [*] bundle.properties 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/OSGI-INF/l10n/bundle.properties?root=Tools_Project&r1=1.14&r2=1.15
(In reply to comment #25) > Patch applied. Thanks for applying. > testNonVirtualDtorInBaseClass2 is still failing Oops. This might be a timeout issue in the JUnit test. I noticed another test within the test suite failing from time to time. The test case passes in the editor. > although I > don't agree with the test that the warning should be reported not just on A but > on the derived classes too. I did that because those classes might be in other translation units. If not doing this the warning is much less visible. I was thinking about the following scenario: Having a class hierarchy (something similar like the test case) with all class definitions in their own file, no virtual method yet. Then a developer adds a virtual method to, let's say class C, with just that file open in the editor. He will be instantly informed that he has to consider the dtor in A as well.
(In reply to comment #27) In the following example there should be warning on A, but not on B: class A { public: ~A(); virtual void m(); }; class B : public A { }; In the following example there should be warning on both, A and B: class A { public: ~A(); virtual void m(); }; class B : public A { virtual void m2(); };
(In reply to comment #28) > In the following example there should be warning on both, A and B: > > class A { > public: > ~A(); > virtual void m(); > }; > > class B : public A { > virtual void m2(); > }; This works here. You might have been running into Bug 333494.
Created attachment 196819 [details] fix5 I have found another false positive. Codan must check if class is abstract as well. Test added and checker fixed.
(In reply to comment #30) Patch submitted. testNonVirtualDtorInBaseClass2 is still wrong since no warnings should be reported for derived classes (lines 7, 11, 13).
(In reply to comment #31) > testNonVirtualDtorInBaseClass2 is still wrong since no warnings > should be reported for derived classes (lines 7, 11, 13). Why do you think they shouldn't be reported? For me this is the expected behaviour. I have tried to explain that in comment #27. It is very useful for me, as an end-user, to do it this way. It think it is useful for others as well. Anyone CC'ed and watching this, what are your opinions?
*** cdt cvs genie on behalf of sprigogin *** Bug 315528 - [fp] Non-virtual destructor diagnostics doesn't take superclass into account. Patch by Patrick Hofer. [*] NonVirtualDestructorCheckerTest.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.core.test/src/org/eclipse/cdt/codan/core/internal/checkers/NonVirtualDestructorCheckerTest.java?root=Tools_Project&r1=1.2&r2=1.3 [*] NonVirtualDestructor.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/codan/org.eclipse.cdt.codan.checkers/src/org/eclipse/cdt/codan/internal/checkers/NonVirtualDestructor.java?root=Tools_Project&r1=1.12&r2=1.13
(In reply to comment #32) > (In reply to comment #31) > > testNonVirtualDtorInBaseClass2 is still wrong since no warnings > > should be reported for derived classes (lines 7, 11, 13). > Why do you think they shouldn't be reported? Because the derived classes didn't do anything wrong except deriving from A, which is bad and is already flagged as such. See also comment #28.
(In reply to comment #34) > Because the derived classes didn't do anything wrong except deriving from A, > which is bad and is already flagged as such. See also comment #28. That's correct. Nevertheless derived D has a problem as well. Then a developer, debugging some weird thing in class D, has to check manually if there are any other related warnings in his projects. Assuming there are hundreds of warnings, this can be tedious. I have worked on larger non perfect code bases and do not rely anymore that things were fixed. Some developers are pretty good in ignoring warnings, and some write buggy code. Have written bugs myself ;-) To sum this up, you're right, the warning is redundant indeed. But warning a little more doesn't harm, does it? If you insist I will remove those warnings.
(In reply to comment #32) > For me this is the expected behaviour. I have tried to explain that in comment > #27. It is very useful for me, as an end-user, to do it this way. It think it > is useful for others as well. > > Anyone CC'ed and watching this, what are your opinions? This scenario sounds reasonable for me, too.
Any progress on fixing this? Should I take over from here? Also, do we reached an agreement on how to treat derived classes with no declared virtual methods? I'd like to hear pros and cons of repeating (or not repeating) the warning in derived classes all the time. See also: bug 326985
(In reply to comment #37) > Any progress on fixing this? Should I take over from here? I regard it as fixed from my side.
Created attachment 198254 [details] more tests, including 326985 Some more problems. One is about how implicit destructors are checked in base classes, the other is for the issue reported in bug 326985 (which I've suggested to be marked a duplicate of this).
Also I'd take Patrick and Axel's side of the issue of reporting errors in all (non-abstract) classes which suffer from this problem, even if they don't introduce any new virtual methods.
What is the status? Is all patches applied?
(In reply to comment #41) > What is the status? Is all patches applied? The last patch by Tomasz has not been applied yet.
(In reply to comment #41) > What is the status? I have downloaded the Indigo release and I still get the warning from the initial comment #0
The following test case still produces a bogus warning: class A { public: virtual ~A(); }; class B : public A { public: ~B(); virtual void m(); friend class C; };
Fixed in cdt_8_0 and master > 20110717.
*** cdt git genie on behalf of 315528 *** Removed an invalid exemption for abstract classes. commit 1feac1a80ae37ea8d80e23da5760e202aa65b498 Bug 351271 - Unused header will be removed from index when saved. Fix without API changes. commit fc4ef1bc10468503d194c5a0b4419fa236e46022 Bug 351271 - Unused header will be removed from index when saved. commit f00afd1e2672a9664a26962a7c78a9592096607a Improved grouping of changes by eliminating a user-visible layer above files. commit f026d893de6ebbc4f27c32aa80a53a14342980af Make setter parameter follow the name style preference. commit 4712cb1e66e212845971fa69f49c14d970100734 Bug 315528 - [fp] Non-virtual destructor diagnostics doesn't take superclass into account. Fix and additional tests including the ones contributed by Tomasz Wesolowski. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=822ae5e4a9b0c02a67f1537f65a7f2edadfcf7f6 [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=1feac1a80ae37ea8d80e23da5760e202aa65b498 [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=fc4ef1bc10468503d194c5a0b4419fa236e46022 [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f00afd1e2672a9664a26962a7c78a9592096607a [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f026d893de6ebbc4f27c32aa80a53a14342980af [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=4712cb1e66e212845971fa69f49c14d970100734
*** cdt git genie on behalf of 315528 *** Removed an invalid exemption for abstract classes. commit 21e056c85659a76f88f4a26de0e0704c9ed1756a Bug 315528 - [fp] Non-virtual destructor diagnostics doesn't take superclass into account. Fix and additional tests including the ones contributed by Tomasz Wesolowski. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=d3264a74180ab0306d5dd217da409bf309031ff5 [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=21e056c85659a76f88f4a26de0e0704c9ed1756a
(In reply to comment #46) It looks like CDT Genie got confused by multiple commits pushed together.
(In reply to comment #48) > (In reply to comment #46) > It looks like CDT Genie got confused by multiple commits pushed together. Is everything resolved now? Can I help somewhere?
(In reply to comment #49) > Is everything resolved now? I hope it is. Notice how simple the code become in the end.
The following code still gives the warning for me... class A { public: virtual void foo() = 0; virtual ~A(); }; class B : public A { public: virtual void foo(); }; class C : public B // <-- "Class 'C' has virtual method 'foo' but non-virtual destructor" { };
(In reply to comment #51) > The following code still gives the warning for me... Which version of CDT do you use? I tried your sample code with 8.0.0, 8.0.1 and 8.1 (master branch) and it worked.
Is this supposed to work with templates? For example I get warning for the following code-snippet: template <typename X> class A { public: virtual void foo() = 0; virtual ~A(); }; template <typename X> class B : public A<X> // <-- "Class 'C@xxxxxxxx' has virtual method 'foo' but non-virtual destructor" { public: virtual void foo(); };
(In reply to comment #52) > Which version of CDT do you use? I tried your sample code with 8.0.0, 8.0.1 and > 8.1 (master branch) and it worked. CDT 8.0.0 release version, didn't try the latest builds. So it may already be fixed, sorry for not checking that. By the way, did you actually copy & paste the code? I noticed that the warning disappears when changing class A { public: virtual void foo() = 0; virtual ~A(); }; to class A { public: virtual ~A(); virtual void foo() = 0; }; i.e., placing the destructor before the foo() method. Quite strange.
