[cfe-commits] r131989 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExprCXX.cpp test/SemaCXX/destructor.cpp

[Nico Weber]

On Wed, Jun 1, 2011 at 10:26 AM, Matthieu Monrocq
<matthieu.monrocq at gmail.com> wrote:
>
>
> 2011/5/30 Eli Friedman <eli.friedman at gmail.com>
>>
>> On Mon, May 30, 2011 at 2:40 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>
>> wrote:
>> > On May 30, 2011, at 2:36 PM, Argyrios Kyrtzidis wrote:
>> >
>> > Hi Nico,
>> > On May 30, 2011, at 12:19 PM, Nico Weber wrote:
>> >
>> > Hi Argyrios and Matthieu,
>> > this warning found a few problems in chromium – thanks! However, it also
>> > finds a few false positives, so I don't think I can turn it on by
>> > default,
>> > which is a bummer.
>> > All of the false positives are of this form:
>> > class SomeInterface {
>> >  public:
>> >   virtual void interfaceMethod() {}  // or = 0;
>> >  protected:
>> >   ~SomeInterface() {}
>> > }
>> > class WorkerClass : public SomeInterface {
>> >  public:
>> >   // many non-virtual functions, but also:
>> >   virtual void interfaceMethod() override { /* do actual work */ }
>> > };
>> > void f() {
>> >   scoped_ptr<WorkerClass> c(new WorkerClass);  // simplified example
>> > }
>> > This is a somewhat standard pattern (see
>> > e.g. http://www.gotw.ca/publications/mill18.htm, "Virtual Question #2").
>> > Do you have any good suggestions how to deal with this case?
>> >
>> > If WorkerClass gets subclassed in the future, deletion in "f()" will be
>> > undefined behavior. Ideally WorkerClass should be marked "final" and
>> > then
>> > there will also be no warning; does this sound reasonable ?
>> >
>> > Um, to be exact, undefined behavior if you delete a WorkerClass *
>> > pointer.
>>
>> Sure, except that there isn't any way to do that outside of C++0x mode. :)
>>
>> -Eli
>
> Hi Nico,
>
> first of all, thanks for field-testing the warning on the chromium code
> base, and I am glad it helped you spot some bugs :)
>
> I've tried to make the warning as tight as possible, to prune as much
> false-positives as possible however there is one difficulty: I only reason
> about the static type. I do not know if Clang has information about the
> dynamic type in the AST. In general it's a hard problem (and requires
> Inter-Procedural Analysis) and out of reach for a "simple" warning. It could
> perhaps be added to the Static Analyzer.
>
> If you can, I would suggest using the "final" attribute:   class WorkerClass
> final: public SomeInterface
> (this could be activated only on the Clang build)
>
> If you cannot, unfortunately I cannot see anything (yet) to prune out this
> case. Perhaps that someone will have a genial idea ?

Follow-up: We decided that we don't want to add "final" to chromium
code, with the argument that it would be used only in comparatively
few cases, so most people wouldn't know "final" exists. And we felt
C++ is complicated enough as is already.

Instead, we're just making the destructor of WorkerClass virtual.
While that's not needed, it makes clang happy. I did this for all
files we build with -Werror (most notably, this includes all chromium
code and all webkit code) and updated our builders to a clang version
that has this warning enabled. I'll keep an eye on how often it
complains about useful things and how often it doesn't.

In my attempt to enable this warning, I fixed 1 real bug (a destructor
should've been virtual but wasn't, and the subclass destructor did
real work), 5 latent bugs (a destructor that should've been virtual
but wasn't, but the subclass didn't have a destructor and no non-POD
members), and added "virtual" to 47 destructors just to make the
warning happy. That's a signal-noise ratio of 12%. I will keep an eye
on how this warning does in practice (i.e. when it turns our build
red, was it for good or bogus reasons?).

For reference, when I initially turned on -Woverride-virtual, I fixed
18 bugs (methods that once were overrides but where the overrides
silently broke due to the superclass changing), and renamed 25 methods
that triggered the warning but weren't actually buggy (but still
confusing, so these changes were still useful). This is a signal-noise
ratio of 72%. That seemed low to me at the time, but the warning has
been proven extremely useful in practice.

I hope this is useful feedback :-)

Nico

ps: Tracking bug for -Wdelete-non-virtual-dtor was
http://crbug.com/84424 , -Woverride-virtual was http://crbug.com/72205