Closed Bug 765381 Opened 12 years ago Closed 12 years ago

Hundreds of -Wdelete-non-virtual-dtor compiler warnings in html/parser

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: gps, Assigned: hsivonen)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file list of warnings
parser/html/nsHtml5AttributeName.cpp and parser/html/nsHtml5ElementName.cpp together account for nearly 1/3 of *all* the compiler warnings when building on Clang SVN HEAD on OS X.

The list of all the warnings in this directory is attached. Most of them are for -Wdelete-non-virtual-dtor.
So I take it that I should just make the destructor virtual even though the subclass doesn't define new fields and doesn't define a destructor?
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> So I take it that I should just make the destructor virtual even though the
> subclass doesn't define new fields and doesn't define a destructor?

That sounds like something that should be fixed in clang.
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> So I take it that I should just make the destructor virtual even though the
> subclass doesn't define new fields and doesn't define a destructor?

Can itself be subclassed? If so a bug could be in there and hence the warning. If not, can you mark the class final? That will fix the warning.
Yeah, there's no way for the compiler to know that the class won't be inherited from elsewhere (unless it's in an anonymous namespace or something...)
Henri, is there a reason that this wouldn't work?
Note that this doesn't actually fix the warnings, it just cuts the number of warnings down to 3.
Attachment #641355 - Flags: feedback?(hsivonen)
Perhaps if the destructor is virtual and we put everything in an anonymous namespace the compiler would be smart enough to inline it?
(In reply to David Zbarsky from comment #6)
> Perhaps if the destructor is virtual and we put everything in an anonymous
> namespace the compiler would be smart enough to inline it?

I can't comment on that. You could also add MOZ_FINAL to the class. See also bug 758992.
So I think what would work, would be to add a flag isPermanent to nsHtml5AttributeName, and merge nsHtml5ReleaseableAttributeName into nsHtml5AttributeName.  Then, make release and cloneAttributeName nonvirtual and condition their behavior on isPermanent.  This shouldn't affect the size of nsHtml5Attribute because the vtable pointer will go away.
For clarity: I don't have an objection to making the destructor virtual. I was just checking to make sure that the current code isn't actually bogus.

Unless there's a really good reason not to make the destructor virtual, let's just make it virtual. I can write the patch now that I'm back from my travels.
(In reply to comment #9)
> Unless there's a really good reason not to make the destructor virtual, let's
> just make it virtual. I can write the patch now that I'm back from my travels.

Yeah, I think that's the right fix here.
Comment on attachment 641355 [details] [diff] [review]
delete entries of AttributeNames/ElementNames, instead of deleting each one separately

Let's just make the destructors virtual and avoid complicating the translation with loop generation fanciness.
Attachment #641355 - Flags: feedback?(hsivonen) → feedback-
Assignee: nobody → hsivonen
Attachment #641355 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 641826 [details] [diff] [review]
Make the destructors virtual, m-c patch

I don't have a clang build set up. Can you check that this successfully makes clang happy?
Attachment #641826 - Flags: review?(gps)
Comment on attachment 641826 [details] [diff] [review]
Make the destructors virtual, m-c patch

It does!  Thanks :-)
Attachment #641826 - Flags: review?(gps) → review+
I went ahead and landed this as it is driving me crazy!  ;-)

Thanks for your patch, Henri!

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7fd2550d3c
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/5c7fd2550d3c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: