<br><br><div class="gmail_quote">2011/5/30 Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</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 Mon, May 30, 2011 at 2:40 PM, Argyrios Kyrtzidis <<a href="mailto:akyrtzi@gmail.com">akyrtzi@gmail.com</a>> 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 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 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 * pointer.<br>
<br>
</div></div>Sure, except that there isn't any way to do that outside of C++0x mode. :)<br>
<font color="#888888"><br>
-Eli<br>
</font></blockquote></div><br>Hi Nico,<br><br>first of all, thanks for field-testing the warning on the chromium code 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 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.<br>
<br>If you can, I would suggest using the "final" attribute:   class WorkerClass 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 case. Perhaps that someone will have a genial idea ?<br>
<br>-- Matthieu<br>