Last Comment Bug 765381 - Hundreds of -Wdelete-non-virtual-dtor compiler warnings in html/parser
: Hundreds of -Wdelete-non-virtual-dtor compiler warnings in html/parser
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
Depends on:
Blocks: clang
  Show dependency treegraph
 
Reported: 2012-06-15 14:51 PDT by Gregory Szorc [:gps]
Modified: 2012-09-21 05:21 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
list of warnings (156.45 KB, text/plain)
2012-06-15 14:51 PDT, Gregory Szorc [:gps]
no flags Details
delete entries of AttributeNames/ElementNames, instead of deleting each one separately (24.71 KB, patch)
2012-07-11 22:10 PDT, David Zbarsky (:dzbarsky)
hsivonen: feedback-
Details | Diff | Splinter Review
Make the destructors virtual, m-c patch (3.64 KB, patch)
2012-07-13 05:09 PDT, Henri Sivonen (:hsivonen)
ehsan: review+
Details | Diff | Splinter Review
Make the destructors virtual, htmlparser patch (3.64 KB, patch)
2012-07-13 05:10 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-06-15 14:51:55 PDT
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.
Comment 1 Henri Sivonen (:hsivonen) 2012-06-16 02:00:05 PDT
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?
Comment 2 Mike Hommey [:glandium] 2012-06-16 02:09:51 PDT
(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.
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-06-16 06:52:40 PDT
(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.
Comment 4 :Ehsan Akhgari 2012-06-18 12:56:47 PDT
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...)
Comment 5 David Zbarsky (:dzbarsky) 2012-07-11 22:10:51 PDT
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.
Comment 6 David Zbarsky (:dzbarsky) 2012-07-11 22:14:42 PDT
Perhaps if the destructor is virtual and we put everything in an anonymous namespace the compiler would be smart enough to inline it?
Comment 7 Gregory Szorc [:gps] 2012-07-11 22:22:04 PDT
(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.
Comment 8 David Zbarsky (:dzbarsky) 2012-07-11 22:30:51 PDT
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.
Comment 9 Henri Sivonen (:hsivonen) 2012-07-11 23:29:31 PDT
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.
Comment 10 :Ehsan Akhgari 2012-07-12 12:37:06 PDT
(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 11 Henri Sivonen (:hsivonen) 2012-07-13 05:08:04 PDT
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.
Comment 12 Henri Sivonen (:hsivonen) 2012-07-13 05:09:13 PDT
Created attachment 641826 [details] [diff] [review]
Make the destructors virtual, m-c patch
Comment 13 Henri Sivonen (:hsivonen) 2012-07-13 05:10:47 PDT
Created attachment 641828 [details] [diff] [review]
Make the destructors virtual, htmlparser patch
Comment 14 Henri Sivonen (:hsivonen) 2012-07-13 05:12:18 PDT
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?
Comment 15 :Ehsan Akhgari 2012-07-13 10:50:39 PDT
Comment on attachment 641826 [details] [diff] [review]
Make the destructors virtual, m-c patch

It does!  Thanks :-)
Comment 16 :Ehsan Akhgari 2012-07-13 12:27:35 PDT
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
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-07-13 20:22:06 PDT
https://hg.mozilla.org/mozilla-central/rev/5c7fd2550d3c
Comment 18 Henri Sivonen (:hsivonen) 2012-09-21 05:21:32 PDT
https://hg.mozilla.org/projects/htmlparser/rev/48f018bb2d2a

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