Last Comment Bug 723501 - Mark MemoryReporter_* final
: Mark MemoryReporter_* final
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-02 06:46 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-02-03 10:54 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mark class final (929 bytes, patch)
2012-02-02 06:46 PST, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-02 06:46:44 PST
Created attachment 593822 [details] [diff] [review]
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) 2012-02-02 09:43:43 PST
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 [:njn] 2012-02-02 13:46:35 PST
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) 2012-02-02 14:02:37 PST
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 [:njn] 2012-02-02 15:09:15 PST
> 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) 2012-02-03 01:18:32 PST
> 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) 2012-02-03 01:19:49 PST
> 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) 2012-02-03 01:24:16 PST
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=06e13280517b
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-02-03 01:25:10 PST
> 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] 2012-02-03 02:05:28 PST
(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) 2012-02-03 02:30:32 PST
> 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] 2012-02-03 02:58:19 PST
(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] 2012-02-03 10:54:05 PST
https://hg.mozilla.org/mozilla-central/rev/06e13280517b

Note You need to log in before you can comment on or make changes to this bug.