2011/6/13 Nico Weber <[email protected]> > On Fri, Jun 10, 2011 at 9:31 AM, Matthieu Monrocq > <[email protected]> wrote: > > > > > > 2011/6/9 Nico Weber <[email protected]> > >> > >> On Wed, Jun 1, 2011 at 10:26 AM, Matthieu Monrocq > >> <[email protected]> wrote: > >> > > >> > > >> > 2011/5/30 Eli Friedman <[email protected]> > >> >> > >> >> On Mon, May 30, 2011 at 2:40 PM, Argyrios Kyrtzidis < > [email protected]> > >> >> 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 > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
