Closed
Bug 1291970
Opened 7 years ago
Closed 7 years ago
Use MOZ_MUST_USE in nsMemoryReporterManager.cpp
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
18.09 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
19.52 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
2.23 KB,
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
Let's use MOZ_MUST_USE as much as is sensible in nsMemoryReporterManager.cpp.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
Attachment #8777633 -
Flags: review?(erahm)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
(Previous patch included an unintentional change.)
Attachment #8777634 -
Flags: review?(erahm)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8777633 -
Attachment is obsolete: true
Attachment #8777633 -
Flags: review?(erahm)
Comment 3•7 years ago
|
||
Comment on attachment 8777634 [details] [diff] [review] Use MOZ_MUST_USE in nsMemoryReporterManager.cpp Review of attachment 8777634 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine, but I'm not sure it's have the effect you want. Either I'm guessing you need another patch that fixes all the bustage from calls to |RegisterWeakMemoryReporter| or tacking |MOZ_MUST_USE| to the definition (vs declaration) doesn't propagate outside of the file? So if the intent is just local r+, if not f+.
Attachment #8777634 -
Flags: review?(erahm) → review+
![]() |
Assignee | |
Comment 4•7 years ago
|
||
I've kept the MOZ_MUST_USE annotations on static functions within the .cpp file, and added some to the header. Thanks for catching the oversight in the previous patch!
Attachment #8779919 -
Flags: review?(erahm)
![]() |
Assignee | |
Comment 5•7 years ago
|
||
This patch adds an assertion that makes sure that the nsCategoryManager is destroyed after the nsMemoryReporterManager, because bad things would happen otherwise. Also, nsCategoryManager uses manual memory management (it's AddRef/Release are hardwired to always return 2 and 1 respectively) so it doesn't matter if we register it as a strong or weak memory reporter. But it's more common to use RegisterWeakMemoryReporter when the argument is |this|, so this patch does that.
Attachment #8779920 -
Flags: review?(erahm)
Comment 6•7 years ago
|
||
Comment on attachment 8779919 [details] [diff] [review] (part 1) - Use MOZ_MUST_USE in nsMemoryReporterManager Review of attachment 8779919 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: xpcom/base/nsIMemoryReporter.idl @@ -472,5 @@ > // the following functions for those components to be registered with the > // manager. > > typedef int64_t (*InfallibleAmountFn)(); > -typedef nsresult (*FallibleAmountFn)(int64_t* aAmount); Is this just extra cleanup of an unused type?
Attachment #8779919 -
Flags: review?(erahm) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8779920 [details] [diff] [review] (part 2) - Tweak nsCategoryManager memory reporter handling Review of attachment 8779920 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine, although I'm not sure what it has to do with this bug :)
Attachment #8779920 -
Flags: review?(erahm) → review+
![]() |
Assignee | |
Comment 8•7 years ago
|
||
> Is this just extra cleanup of an unused type?
Yes.
![]() |
Assignee | |
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6612a73409a9029aff2461b9ab9f451b4677f51d Bug 1291970 (part 1) - Use MOZ_MUST_USE in nsMemoryReporterManager. r=erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/b8117f4a3659120291ecbfcc07650fed632dda02 Bug 1291970 (part 2) - Tweak nsCategoryManager memory reporter handling. r=erahm.
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6612a73409a9 https://hg.mozilla.org/mozilla-central/rev/b8117f4a3659
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•