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

Matthieu Monrocq matthieu.monrocq at gmail.com
Mon Jun 13 04:01:02 PDT 2011


2011/6/13 Nico Weber <thakis at chromium.org>

> On Fri, Jun 10, 2011 at 9:31 AM, Matthieu Monrocq
> <matthieu.monrocq at gmail.com> wrote:
> >
> >
> > 2011/6/9 Nico Weber <thakis at chromium.org>
> >>
> >> 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
> >
> > Thank you very much!
> >
> > It is very useful indeed, based on your experience I guess we will not
> > activate this warning by default any time soon.
>
> What do you mean with "by default"? For me, that means "in -Wall", and
> It looks like this warning is currently in -Wmost. (I'm not
> complaining or saying that that should change, I just want to
> understand what you mean.)
>
> Nico
>
> I meant it is "DefaultIgnore"d, that is only activated if you request it
with a group flag. I was under the impression that a number of warnings in
Clang were reported even when no warning flag at all were passed to the
compiler.

-- Matthieu.


> > "final" would probably have
> > allowed a number of optimizations too (devirtualizations of functions
> calls)
> > but I can understand the reluctance to introduce supplementary keywords
> in
> > an already convoluted language :)
> >
> > -- Matthieu
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110613/034e0b44/attachment.html>


More information about the cfe-commits mailing list