<br><br><div class="gmail_quote">2011/6/13 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;">
On Fri, Jun 10, 2011 at 9:31 AM, Matthieu Monrocq<br>
<div><div></div><div class="h5"><<a href="mailto:matthieu.monrocq@gmail.com">matthieu.monrocq@gmail.com</a>> wrote:<br>
><br>
><br>
> 2011/6/9 Nico Weber <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>><br>
>><br>
>> 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<br>
>> >> > 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<br>
>> >> > #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<br>
>> >> > 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>
>> >><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<br>
>> > 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<br>
>> > could<br>
>> > perhaps be added to the Static Analyzer.<br>
>> ><br>
>> > If you can, I would suggest using the "final" attribute: class<br>
>> > 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<br>
>> > this<br>
>> > case. Perhaps that someone will have a genial idea ?<br>
>><br>
>> 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>
>><br>
>> Nico<br>
>><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>
><br>
> Thank you very much!<br>
><br>
> It is very useful indeed, based on your experience I guess we will not<br>
> activate this warning by default any time soon.<br>
<br>
</div></div>What do you mean with "by default"? For me, that means "in -Wall", and<br>
It looks like this warning is currently in -Wmost. (I'm not<br>
complaining or saying that that should change, I just want to<br>
understand what you mean.)<br>
<font color="#888888"><br>
Nico<br>
</font><div><div></div><div class="h5"><br></div></div></blockquote><div>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.<br>
<br>-- Matthieu.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div><div class="h5">
> "final" would probably have<br>
> allowed a number of optimizations too (devirtualizations of functions calls)<br>
> but I can understand the reluctance to introduce supplementary keywords in<br>
> an already convoluted language :)<br>
><br>
> -- Matthieu<br>
><br>
</div></div></blockquote></div><br>