1169974 - DevTools.h:278:16: warning: 'writeNode' overrides a member function but is not marked 'override' [-Winconsistent-missing-override] (same for "writeMetadata")

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Daniel Holbert [:dholbert]]

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

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Nicholas Nethercote [inactive]]

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

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 8

[Nicholas Nethercote [inactive]]

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

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Tolerate inconsistent-missing-override warnings for MOCK_METHOD2 macro from gtests

Comment 10

[Nicholas Nethercote [inactive]]

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

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Nicholas Nethercote [inactive]]

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

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/731200b5ab4e

Comment 16

[Daniel Holbert [:dholbert]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/731200b5ab4e

Comment 18

[Daniel Holbert [:dholbert]]

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=..."?

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: part 2: make clang suppress the warnings (instead of just allowing them)

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

Comment 20

[Nicholas Nethercote [inactive]]

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.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2056cb9f608d

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2056cb9f608d