Closed Bug 1125632 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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]
}
Attached patch fix: add MOZ_OVERRIDE (obsolete) — Splinter Review
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8554281 - Flags: review?(n.nethercote)
(sorry, previous patch version only had 3 lines of context, due to a stale local hgrc tweak. bumped it up to 8)
Attachment #8554281 - Attachment is obsolete: true
Attachment #8554281 - Flags: review?(n.nethercote)
Attachment #8554283 - Flags: review?(n.nethercote)
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?
Attachment #8554283 - Flags: review?(n.nethercote) → review+
(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.
(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)
Makes sense. Thanks for the extra info.
https://hg.mozilla.org/mozilla-central/rev/6ff2bc5f6f0c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(dholbert)
Resolution: FIXED → ---
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.)
Flags: needinfo?(dholbert)
https://hg.mozilla.org/mozilla-central/rev/cb8cbda065c6
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.