1125632 - SystemMemoryReporter.cpp:161:14: warning: 'CollectReports' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel Holbert [:dholbert]]

Clang 3.6 build warning:
{
xpcom/base/SystemMemoryReporter.cpp:161:14: warning: 'CollectReports' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
}

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix: add MOZ_OVERRIDE

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (now with more lines of context)

(sorry, previous patch version only had 3 lines of context, due to a stale local hgrc tweak. bumped it up to 8)

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8554283 [details] [diff] [review]
fix v2 (now with more lines of context)

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

This seems fine but I'm surprised you're only getting one warning... by my count, there are 10 more similar nsIMemoryReporter subclasses in nsMemoryReporterManager.cpp alone. Not all of them build on all platforms, but some done. And then there are dozens more cases like this throughout the codebase. Why doesn't clang complain about them?

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> This seems fine but I'm surprised you're only getting one warning...

Only one warning for this file (SystemMemoryReporter.cpp).

> by my
> count, there are 10 more similar nsIMemoryReporter subclasses in
> nsMemoryReporterManager.cpp alone.

I'm not sure if I get warnings for any of those, but note that ehsan labeled a bunch of those as MOZ_OVERRIDE in http://hg.mozilla.org/mozilla-central/diff/c3fc371864af/xpcom/base/nsMemoryReporterManager.cpp

> And then there are dozens more cases like this throughout the
> codebase. Why doesn't clang complain about them?

This is a new warning in clang trunk (version 3.6). It does complain about them quite a bit, but (a) very few people use this version of clang, and (b) ehsan and I (and probably others) have been knocking them out in bug 1117034 & dependencies.

Comment 5

[Daniel Holbert [:dholbert]]

(You're probably wondering -- we hit this warning frequently enough where you can't currently build with --enable-warnings-as-errors, unless you suppress this warning -- per bug 1117034 comment 2)

Comment 6

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff2bc5f6f0c

Comment 7

[Nicholas Nethercote [inactive]]

Makes sense. Thanks for the extra info.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ff2bc5f6f0c

Comment 9

[Daniel Holbert [:dholbert]]

Er, looks like I somehow annotated one line of nsMemoryReporterManager.cpp here, instead of annotating the line of SystemMemoryReporter.cpp that was pointed to by the warning in comment 0.

That explains why njn was confused in comment 3.  :)

I'll push a followup patch tomorrow morning to annotate the CollectReports() method that's *actually* in SystemMemoryReporter.cpp, along with the rest of the ones in nsMemoryReporterManager.cpp.

Comment 10

[Daniel Holbert [:dholbert]]

Landed that followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8cbda065c6

(Didn't bother with review, to save everyone time, because it's a mechanical change which is a direct extension of the rest of this bug, and already has implicit approval from njn in comment 3 here.)

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cb8cbda065c6