Suppress -Wdelete-non-virtual-dtor warning in nsAtomTable.cpp (and mark xpcom/ds as FAIL_ON_WARNINGS)

RESOLVED FIXED in Firefox 28

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

Trunk
mozilla28
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Assignee)

Comment 1

5 years ago
Comment on attachment 8341570 [details] [diff] [review]
part-1-suppress-clang-warning.patch

I chose to suppress this warning for clang and gcc instead of making AtomImpl's destructor virtual because bug 229875 (from 2004) went out of its way to remove unnecessary virtual destructors and added a comment explaining why:

> // We don't need a virtual destructor here because PermanentAtomImpl
> // deletions aren't handled through Release().
> ~AtomImpl(); 

https://github.com/mozilla/gecko-dev/commit/a10a5e838374e242a2ea5ff6383a77916aba0499#diff-7dc7ebae3c7e5754356b39bb0e7e0e33R54
Attachment #8341570 - Flags: review?(doug.turner)
(Assignee)

Comment 2

5 years ago
Created attachment 8341575 [details] [diff] [review]
part-2-fail-on-warnings.patch
Attachment #8341575 - Flags: review?(doug.turner)
No longer blocks: 557566
Depends on: 557566

Comment 3

5 years ago
Comment on attachment 8341570 [details] [diff] [review]
part-1-suppress-clang-warning.patch

Review of attachment 8341570 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing since I've looked into this before.

::: xpcom/ds/nsAtomTable.cpp
@@ +23,5 @@
>  #include "nsUnicharUtils.h"
>  
>  using namespace mozilla;
>  
> +#if defined(__clang__) || defined(__GNUC__)

Just do #ifdef __GNUC__.  Clang emulates that.
Attachment #8341570 - Flags: review?(doug.turner) → review+

Updated

5 years ago
Attachment #8341575 - Flags: review?(doug.turner) → review+
https://hg.mozilla.org/mozilla-central/rev/8002c2110210
https://hg.mozilla.org/mozilla-central/rev/96d025fe7fb4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
On Ubuntu 12.04 LTS 64bit, which is using GCC 4.6.2, I get:

 0:05.98 In file included from /hack/mozilla-central/obj-firefox-debug/xpcom/ds/Unified_cpp_xpcom_ds0.cpp:29:0:
 0:05.98 /hack/mozilla-central/xpcom/ds/nsAtomTable.cpp:28:32: error: unknown option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]

Comment 7

5 years ago
(In reply to comment #6)
> On Ubuntu 12.04 LTS 64bit, which is using GCC 4.6.2, I get:
> 
>  0:05.98 In file included from
> /hack/mozilla-central/obj-firefox-debug/xpcom/ds/Unified_cpp_xpcom_ds0.cpp:29:0:
>  0:05.98 /hack/mozilla-central/xpcom/ds/nsAtomTable.cpp:28:32: error: unknown
> option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]

That's weird: <http://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Diagnostic-Pragmas.html>
(Assignee)

Comment 8

5 years ago
I suspect the "unknown option" error refers to the "-Wdelete-non-virtual-dtor" flag, which appears to have been added in gcc 4.7:

http://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/C_002b_002b-Dialect-Options.html
http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/C_002b_002b-Dialect-Options.html

Benoit: Can gcc 4.6 compile nsAtomTable.cpp if you use the pragma check below?

  #ifdef __GNUC__
  #  if MOZ_GCC_VERSION_AT_LEAST(4, 7, 0)
  #    pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor"
  #  endif
  #endif
Flags: needinfo?(bjacob)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> (In reply to comment #6)
> > On Ubuntu 12.04 LTS 64bit, which is using GCC 4.6.2, I get:
> > 
> >  0:05.98 In file included from
> > /hack/mozilla-central/obj-firefox-debug/xpcom/ds/Unified_cpp_xpcom_ds0.cpp:29:0:
> >  0:05.98 /hack/mozilla-central/xpcom/ds/nsAtomTable.cpp:28:32: error: unknown
> > option after ‘#pragma GCC diagnostic’ kind [-Werror=pragmas]
> 
> That's weird:
> <http://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Diagnostic-Pragmas.html>

why? that page (and the one for gcc 4.8.2) mention nothing about what happens if you use an invalid warning flag, and imho yelling at you is the sane thing to do especially if the user asked to be yelled at for unknown command line options.
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> why? that page (and the one for gcc 4.8.2) mention nothing about what
> happens if you use an invalid warning flag

(I'm guessing ehsan just misread the warning (error) as saying that "#pragma GCC diagnostic ignored" was *itself* unrecognized, and that URL he linked to says it should be recognized.)

Comment 11

5 years ago
(In reply to comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > why? that page (and the one for gcc 4.8.2) mention nothing about what
> > happens if you use an invalid warning flag
> 
> (I'm guessing ehsan just misread the warning (error) as saying that "#pragma
> GCC diagnostic ignored" was *itself* unrecognized, and that URL he linked to
> says it should be recognized.)

Yes indeed.
(In reply to Chris Peterson (:cpeterson) from comment #8)
> I suspect the "unknown option" error refers to the
> "-Wdelete-non-virtual-dtor" flag, which appears to have been added in gcc
> 4.7:
> 
> http://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/C_002b_002b-Dialect-Options.html
> http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/C_002b_002b-Dialect-Options.html
> 
> Benoit: Can gcc 4.6 compile nsAtomTable.cpp if you use the pragma check
> below?
> 
>   #ifdef __GNUC__
>   #  if MOZ_GCC_VERSION_AT_LEAST(4, 7, 0)
>   #    pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor"
>   #  endif
>   #endif

Yes, that fixes the build for me.
Flags: needinfo?(bjacob)
(In reply to Chris Peterson (:cpeterson) from comment #8)
> I suspect the "unknown option" error refers to the
> "-Wdelete-non-virtual-dtor" flag, which appears to have been added in gcc
> 4.7:
> 
> http://gcc.gnu.org/onlinedocs/gcc-4.6.0/gcc/C_002b_002b-Dialect-Options.html
> http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/C_002b_002b-Dialect-Options.html
> 
> Benoit: Can gcc 4.6 compile nsAtomTable.cpp if you use the pragma check
> below?
> 
>   #ifdef __GNUC__
>   #  if MOZ_GCC_VERSION_AT_LEAST(4, 7, 0)
>   #    pragma GCC diagnostic ignored "-Wdelete-non-virtual-dtor"
>   #  endif
>   #endif

If you're going to commit this, you want:

#if MOZ_IS_GCC == 1
#  if MOZ_GCC_VERSION_AT_LEAST
...

since MOZ_GCC_VERSION_AT_LEAST is not defined on non-GCC compilers, and clang defines __GNUC__.
(Assignee)

Comment 14

5 years ago
Created attachment 8343887 [details] [diff] [review]
gcc-4-7-pragma.patch

Check for either clang or gcc 4.7. This is ugly. :(
Attachment #8343887 - Flags: review?(ehsan)

Comment 15

5 years ago
Comment on attachment 8343887 [details] [diff] [review]
gcc-4-7-pragma.patch

Review of attachment 8343887 [details] [diff] [review]:
-----------------------------------------------------------------

Please #include "mozilla/Compiler.h" explicitly, otherwise this will silently break when that include is removed from whatever header it's coming from now.  r=me with that.
Attachment #8343887 - Flags: review?(ehsan) → review+
Duplicate of this bug: 947380
(Assignee)

Comment 17

5 years ago
Landed with #include "mozilla/Compiler.h":

https://hg.mozilla.org/integration/mozilla-inbound/rev/b183f613840c

Comment 18

5 years ago
Is there an ETA for Attachment #8343887 [details] [diff] turning up in mozilla-central master branch?
(Referring to bug 947380)
(Assignee)

Comment 19

5 years ago
(In reply to Nigel Stewart from comment #18)
> Is there an ETA for Attachment #8343887 [details] [diff] [diff] turning up in
> mozilla-central master branch?

Nigel: this Linux fix just landed on mozilla-inbound, so mozilla-central should get the fix later today. I don't know how long it takes for changes to hg.mozilla.org's mozilla-central to migrate to the mozilla-central repo on GitHub.
status-firefox28: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.