Closed Bug 723501 Opened 12 years ago Closed 12 years ago

Mark MemoryReporter_* final

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: espindola, Assigned: espindola)

Details

Attachments

(1 file)

Attached patch mark class finalSplinter Review
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.
Attachment #593822 - Flags: review?(justin.lebar+bug)
Didn't we decide that this was a benign warning?  Can we simply quiet it with -Wno-delete-non-virtual-dtor?
I'm ok with the MOZ_FINAL declaration, I don't see why you'd want to do sub-classing here.
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.  :)
Attachment #593822 - Flags: review?(justin.lebar+bug) → review+
> 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.
> 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.
> 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.
> 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.
(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.
> 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.
(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...
https://hg.mozilla.org/mozilla-central/rev/06e13280517b
Assignee: nobody → respindola
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: