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.

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.

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 on attachment 641826 [details] [diff] [review] Make the destructors virtual, m-c patch It does! Thanks :-)

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