<br><br><div class="gmail_quote">2011/6/9 Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5">On Wed, Jun 1, 2011 at 10:26 AM, Matthieu Monrocq<br>
<<a href="mailto:matthieu.monrocq@gmail.com">matthieu.monrocq@gmail.com</a>> wrote:<br>
><br>
><br>
> 2011/5/30 Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
>><br>
>> On Mon, May 30, 2011 at 2:40 PM, Argyrios Kyrtzidis <<a href="mailto:akyrtzi@gmail.com">akyrtzi@gmail.com</a>><br>
>> wrote:<br>
>> > On May 30, 2011, at 2:36 PM, Argyrios Kyrtzidis wrote:<br>
>> ><br>
>> > Hi Nico,<br>
>> > On May 30, 2011, at 12:19 PM, Nico Weber wrote:<br>
>> ><br>
>> > Hi Argyrios and Matthieu,<br>
>> > this warning found a few problems in chromium – thanks! However, it also<br>
>> > finds a few false positives, so I don't think I can turn it on by<br>
>> > default,<br>
>> > which is a bummer.<br>
>> > All of the false positives are of this form:<br>
>> > class SomeInterface {<br>
>> >  public:<br>
>> >   virtual void interfaceMethod() {}  // or = 0;<br>
>> >  protected:<br>
>> >   ~SomeInterface() {}<br>
>> > }<br>
>> > class WorkerClass : public SomeInterface {<br>
>> >  public:<br>
>> >   // many non-virtual functions, but also:<br>
>> >   virtual void interfaceMethod() override { /* do actual work */ }<br>
>> > };<br>
>> > void f() {<br>
>> >   scoped_ptr<WorkerClass> c(new WorkerClass);  // simplified example<br>
>> > }<br>
>> > This is a somewhat standard pattern (see<br>
>> > e.g. <a href="http://www.gotw.ca/publications/mill18.htm" target="_blank">http://www.gotw.ca/publications/mill18.htm</a>, "Virtual Question #2").<br>
>> > Do you have any good suggestions how to deal with this case?<br>
>> ><br>
>> > If WorkerClass gets subclassed in the future, deletion in "f()" will be<br>
>> > undefined behavior. Ideally WorkerClass should be marked "final" and<br>
>> > then<br>
>> > there will also be no warning; does this sound reasonable ?<br>
>> ><br>
>> > Um, to be exact, undefined behavior if you delete a WorkerClass *<br>
>> > pointer.<br>
>><br>
>> Sure, except that there isn't any way to do that outside of C++0x mode. :)<br>
>><br>
>> -Eli<br>
><br>
> Hi Nico,<br>
><br>
> first of all, thanks for field-testing the warning on the chromium code<br>
> base, and I am glad it helped you spot some bugs :)<br>
><br>
> I've tried to make the warning as tight as possible, to prune as much<br>
> false-positives as possible however there is one difficulty: I only reason<br>
> about the static type. I do not know if Clang has information about the<br>
> dynamic type in the AST. In general it's a hard problem (and requires<br>
> Inter-Procedural Analysis) and out of reach for a "simple" warning. It could<br>
> perhaps be added to the Static Analyzer.<br>
><br>
> If you can, I would suggest using the "final" attribute:   class WorkerClass<br>
> final: public SomeInterface<br>
> (this could be activated only on the Clang build)<br>
><br>
> If you cannot, unfortunately I cannot see anything (yet) to prune out this<br>
> case. Perhaps that someone will have a genial idea ?<br>
<br>
</div></div>Follow-up: We decided that we don't want to add "final" to chromium<br>
code, with the argument that it would be used only in comparatively<br>
few cases, so most people wouldn't know "final" exists. And we felt<br>
C++ is complicated enough as is already.<br>
<br>
Instead, we're just making the destructor of WorkerClass virtual.<br>
While that's not needed, it makes clang happy. I did this for all<br>
files we build with -Werror (most notably, this includes all chromium<br>
code and all webkit code) and updated our builders to a clang version<br>
that has this warning enabled. I'll keep an eye on how often it<br>
complains about useful things and how often it doesn't.<br>
<br>
In my attempt to enable this warning, I fixed 1 real bug (a destructor<br>
should've been virtual but wasn't, and the subclass destructor did<br>
real work), 5 latent bugs (a destructor that should've been virtual<br>
but wasn't, but the subclass didn't have a destructor and no non-POD<br>
members), and added "virtual" to 47 destructors just to make the<br>
warning happy. That's a signal-noise ratio of 12%. I will keep an eye<br>
on how this warning does in practice (i.e. when it turns our build<br>
red, was it for good or bogus reasons?).<br>
<br>
For reference, when I initially turned on -Woverride-virtual, I fixed<br>
18 bugs (methods that once were overrides but where the overrides<br>
silently broke due to the superclass changing), and renamed 25 methods<br>
that triggered the warning but weren't actually buggy (but still<br>
confusing, so these changes were still useful). This is a signal-noise<br>
ratio of 72%. That seemed low to me at the time, but the warning has<br>
been proven extremely useful in practice.<br>
<br>
I hope this is useful feedback :-)<br>
<font color="#888888"><br>
Nico<br>
</font><br>
ps: Tracking bug for -Wdelete-non-virtual-dtor was<br>
<a href="http://crbug.com/84424" target="_blank">http://crbug.com/84424</a> , -Woverride-virtual was <a href="http://crbug.com/72205" target="_blank">http://crbug.com/72205</a><br>
</blockquote></div><br>Thank you very much!<br><br>It is very useful indeed, based on your experience I guess we will not activate this warning by default any time soon. "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 :)<br>
<br>-- Matthieu<br>