New build warnings, with clang 3.7 (in a non-FAIL_ON_WARNINGS directory, so not fatal): { In file included from $SRC/toolkit/devtools/server/tests/gtest/DeserializedNodeUbiNodes.cpp:10: 19:16.05 Warning: -Winconsistent-missing-override in $SRC/toolkit/devtools/server/tests/gtest/DevTools.h: 'writeNode' overrides a member function but is not marked 'override' $SRC/toolkit/devtools/server/tests/gtest/DevTools.h:278:16: warning: 'writeNode' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] MOCK_METHOD2(writeNode, bool(const JS::ubi::Node&, CoreDumpWriter::EdgePolicy)); ^ ../../../../../dist/include/gmock/gmock-generated-function-mockers.h:634:49: note: expanded from macro 'MOCK_METHOD2' #define MOCK_METHOD2(m, F) GMOCK_METHOD2_(, , , m, F) ^ ../../../../../dist/include/gmock/gmock-generated-function-mockers.h:380:27: note: expanded from macro 'GMOCK_METHOD2_' GMOCK_RESULT_(tn, F) ct Method(GMOCK_ARG_(tn, F, 1) gmock_a1, \ ^ ../../../../../dist/include/mozilla/devtools/ChromeUtils.h:42:16: note: overridden virtual function is here virtual bool writeNode(const JS::ubi::Node& node, ^ In file included from $OBJ/toolkit/devtools/server/tests/gtest/Unified_cpp_server_tests_gtest0.cpp:2: In file included from $SRC/toolkit/devtools/server/tests/gtest/DeserializedNodeUbiNodes.cpp:10: Warning: -Winconsistent-missing-override in $SRC/toolkit/devtools/server/tests/gtest/DevTools.h: 'writeMetadata' overrides a member function but is not marked 'override' $SRC/toolkit/devtools/server/tests/gtest/DevTools.h:279:16: warning: 'writeMetadata' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] MOCK_METHOD1(writeMetadata, bool(uint64_t)); ^ ../../../../../dist/include/gmock/gmock-generated-function-mockers.h:633:49: note: expanded from macro 'MOCK_METHOD1' #define MOCK_METHOD1(m, F) GMOCK_METHOD1_(, , , m, F) ^ ../../../../../dist/include/gmock/gmock-generated-function-mockers.h:364:27: note: expanded from macro 'GMOCK_METHOD1_' GMOCK_RESULT_(tn, F) ct Method(GMOCK_ARG_(tn, F, 1) gmock_a1) constness { \ ^ ../../../../../dist/include/mozilla/devtools/ChromeUtils.h:32:16: note: overridden virtual function is here virtual bool writeMetadata(uint64_t timestamp) = 0; ^ } This is for this chunk of code (lines 278 & 279): > 273 // A mock `Writer` class to be used with testing `WriteHeapGraph`. > 274 class MockWriter : public CoreDumpWriter > 275 { > 276 public: > 277 virtual ~MockWriter() override { } > 278 MOCK_METHOD2(writeNode, bool(const JS::ubi::Node&, CoreDumpWriter::EdgePolicy)); > 279 MOCK_METHOD1(writeMetadata, bool(uint64_t)); > 280 }; http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/gtest/DevTools.h#273 ...added in "part 5" of bug 1024774. Assuming these methods are indeed intended to override, we need to mark them as such (or suffer buildspew in newer clang versions). I'm not sure what the easiest way is to do that, using these gtest macros...

Google seems to have run into this issue w/ MOCK_METHOD as well, last year: > #1: "MOCK_METHOD appears to not add override, so not sure what to do about those." [...] > #19: Filed internal b/18208154 for the gmock thing > #20: We can't fix all instances of this warning at the moment (because of gmock [...] > Blockedon: googlemock:157 https://code.google.com/p/chromium/issues/detail?id=428099

And the "googlemock:157" issue is: https://code.google.com/p/googlemock/issues/detail?id=157

Oh yeah, I came across this was and was also stymied by the macros.

I bump into this via my MOCK_METHOD in gtest. What's the best way to deal this? Just ignore the warning for now?

(I'm not sure anyone here knows how best to deal with it; ignoring it is all we've got right now, I think. Maybe we should suppress this build warning in gtest directories, if that's easy to do, so the compiler can do the ignoring for us.)

(In reply to Daniel Holbert [:dholbert] from comment #5) > (I'm not sure anyone here knows how best to deal with it; ignoring it is all > we've got right now, I think. Maybe we should suppress this build warning in > gtest directories, if that's easy to do, so the compiler can do the ignoring > for us.) +1, we should just ignore for gtests.

For the record, I just allow warning in my gtest in [1] so the warning is discoverable. If it's too annoying, we could hide the warning by add -Wno-inconsistent-missing-override in CXXFLAGS. [1] https://dxr.mozilla.org/mozilla-central/rev/197af2fb7e29ff8e4b3b6ced723b6172e954e17d/layout/base/gtest/moz.build#15

I'm doing a bunch of these right now, so I can include this one too.

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62d444b91e04

Comment on attachment 8664055 [details] [diff] [review] Tolerate inconsistent-missing-override warnings for MOCK_METHOD2 macro from gtests Review of attachment 8664055 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Comment on attachment 8664055 [details] [diff] [review] Tolerate inconsistent-missing-override warnings for MOCK_METHOD2 macro from gtests >+++ b/toolkit/devtools/heapsnapshot/tests/gtest/moz.build >+# THE MOCK_METHOD2 macro from gtest triggers this clang warning and it's hard >+# to work around, so we just ignore it. >+if CONFIG['CLANG_CXX']: >+ CXXFLAGS += ['-Wno-error=inconsistent-missing-override'] As I recall, clang versions which don't support a warning will error out if they're passed it on the command line. Do you know how far back clang supports this particular warning? (Hopefully it was introduced further back than our newest supported-for-building clang version.)

> As I recall, clang versions which don't support a warning will error out if > they're passed it on the command line. Looks like it's a warning by default. I just tried "clang -Wfoo a.c" and got this: warning: unknown warning option '-Wfoo' [-Wunknown-warning-option] > Do you know how far back clang supports this particular warning? 3.6, I think. I can't find mention of it in any release notes but the timing of 3.6's release (March 2015) matches up with bug reports that mention this new warning (e.g. bug 1117034).

I asked glandium about this. Turns out we use -Wno-unknown-warning-option with clang specifically so that passing new warning flags to older versions doesn't cause a problem (see build/autoconf/compiler-opts.m4). So I think we're good to go here.

(In reply to Nicholas Nethercote [:njn] from comment #14) > I asked glandium about this. Turns out we use -Wno-unknown-warning-option > with clang specifically so that passing new warning flags to older versions > doesn't cause a problem (see build/autoconf/compiler-opts.m4). That is excellent news. Thanks for fixing this!

Comment on attachment 8664055 [details] [diff] [review] Tolerate inconsistent-missing-override warnings for MOCK_METHOD2 macro from gtests >+++ b/layout/base/gtest/moz.build >-ALLOW_COMPILER_WARNINGS = True >+# THE MOCK_METHOD2 macro from gtest triggers this clang warning and it's hard >+# to work around, so we just ignore it. >+if CONFIG['CLANG_CXX']: >+ CXXFLAGS += ['-Wno-error=inconsistent-missing-override'] Hmm, so this changed us from allowing all warnings to just allowing this one warning. That's a good change -- but it doesn't really affect our current situation here. We still spam about these warnings -- I'm still seeing output like in comment 0. Per comment 4, 5, 6, I thought the plan here was silence these warnings, not just to allow them in a more targeted way. Perhaps we can take a "part 2" here which upgrades us to "-Wno-inconsistent-missing-override" instead of "-Wno-error=..."?

I think this should do it. Bumping needinfo? to r?.

Comment on attachment 8665673 [details] [diff] [review] part 2: make clang suppress the warnings (instead of just allowing them) Review of attachment 8665673 [details] [diff] [review]: ----------------------------------------------------------------- It's true, I hijacked this bug to be about getting rid of the ALLOW_COMPILER_WARNINGS flag rather than silencing the warning. r=me.