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

RESOLVED FIXED in mozilla16

Status

()

Core
HTML: Parser
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: hsivonen)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 633686 [details]
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.
(Assignee)

Comment 1

5 years ago
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...)
Created attachment 641355 [details] [diff] [review]
delete entries of AttributeNames/ElementNames, instead of deleting each one separately

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?
(Reporter)

Comment 7

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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)

Comment 12

5 years ago
Created attachment 641826 [details] [diff] [review]
Make the destructors virtual, m-c patch
Assignee: nobody → hsivonen
Attachment #641355 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
Created attachment 641828 [details] [diff] [review]
Make the destructors virtual, htmlparser patch
(Assignee)

Comment 14

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/projects/htmlparser/rev/48f018bb2d2a
You need to log in before you can comment on or make changes to this bug.