[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]

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 ?

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110601/8dd2671c/attachment.html>