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

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8002c2110210 https://hg.mozilla.org/integration/mozilla-inbound/rev/96d025fe7fb4

https://hg.mozilla.org/mozilla-central/rev/8002c2110210 https://hg.mozilla.org/mozilla-central/rev/96d025fe7fb4

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]

(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>

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

(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.)

(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.

(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__.

Check for either clang or gcc 4.7. This is ugly. :(

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.

Landed with #include "mozilla/Compiler.h": https://hg.mozilla.org/integration/mozilla-inbound/rev/b183f613840c

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

(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.