723501 - Mark MemoryReporter_* final

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: mark class final

Currently builds with --enable-warnings-as-errors are broken with clang because of the warning/error

error: delete called on 'fooBar' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]

This patch fixes one of them.

Comment 1

[Justin Lebar (not reading bugmail)]

Didn't we decide that this was a benign warning?  Can we simply quiet it with -Wno-delete-non-virtual-dtor?

Comment 2

[Nicholas Nethercote [inactive]]

I'm ok with the MOZ_FINAL declaration, I don't see why you'd want to do sub-classing here.

Comment 3

[Justin Lebar (not reading bugmail)]

Okay, this isn't worth arguing about.

FWIW, Rafael, I think starting with a small, curated list of -Werror's and building up from there would be more useful than trying to fix every single warning that the compiler happens to generate.  There's a reason that many of these are not errors.

But I keep saying this and I've gotten zero traction from the warnings-as-errors folks, so I guess I should stop.  :)

Comment 4

[Nicholas Nethercote [inactive]]

> But I keep saying this and I've gotten zero traction from the
> warnings-as-errors folks, so I guess I should stop.  :)

For GCC, the current C++ warning set is pretty good.  Bug 711895 will make it better.  -Winitialized is the only one that's really problematic -- it finds real problems, but also has lots of false positives.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> But I keep saying this and I've gotten zero traction from the
> warnings-as-errors folks, so I guess I should stop.  :)

This waring found at least one real bug so far and quiet a bit of unclear code. If there is a delete of a class with virtual methods, the options are

* The class is final
* The class has a virtual destructor
* We incorrectly don't call the destructor on the bases.
* The class knows that all is derived class don't have anything interesting to destroy

The only valid (but *very* strange) use case would be the last one, so this is one of the warnings with the best signal/noise ratio I have seen and would be really sad to have to shut it down.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> For GCC, the current C++ warning set is pretty good.  Bug 711895 will make
> it better.  -Winitialized is the only one that's really problematic -- it
> finds real problems, but also has lots of false positives.

Interesting. Clang has much better -Winitialized, we should probably enable it with clang, but so far I am assigned only to get the build going again.

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=06e13280517b

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Interesting. Clang has much better -Winitialized, we should probably enable
> it with clang, but so far I am assigned only to get the build going again.

In fact, we should juts use -Wall -Werror and fix clang if it has a noisy waring.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #5)
> > But I keep saying this and I've gotten zero traction from the
> > warnings-as-errors folks, so I guess I should stop.  :)
> 
> This waring found at least one real bug so far and quiet a bit of unclear
> code. If there is a delete of a class with virtual methods, the options are
> 
> * The class is final
> * The class has a virtual destructor
> * We incorrectly don't call the destructor on the bases.
> * The class knows that all is derived class don't have anything interesting
> to destroy
> 
> The only valid (but *very* strange) use case would be the last one, so this
> is one of the warnings with the best signal/noise ratio I have seen and
> would be really sad to have to shut it down.

The last case happens all the time in XPCOM Release methods.  GCC 4.7+ gets quite a bit noisier with -Wdelete-non-virtual-dtor.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> The last case happens all the time in XPCOM Release methods.  GCC 4.7+ gets
> quite a bit noisier with -Wdelete-non-virtual-dtor.

Is delete called on those classes? Clang only warn when it sees a delete.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #10)
> > The last case happens all the time in XPCOM Release methods.  GCC 4.7+ gets
> > quite a bit noisier with -Wdelete-non-virtual-dtor.
> 
> Is delete called on those classes? Clang only warn when it sees a delete.

See nsISupportsImpl.h:NS_IMPL_RELEASE_WITH_DESTROY and the various 'delete this' callers in that file.  It's certainly possible there's a bug in Clang or GCC here...

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/06e13280517b