Closed
Bug 1268772
Opened 8 years ago
Closed 8 years ago
Use MOZ_MUST_USE more in xpcom/ds/
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(5 files, 2 obsolete files)
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
There are a bunch of places in xpcom/ds/ where MOZ_MUST_USE can be added, and it finds some missing failure checks along the way.
Assignee | ||
Comment 1•8 years ago
|
||
It's always NS_OK and none of the call sites look at it.
Attachment #8746946 -
Flags: review?(erahm)
Assignee | ||
Comment 2•8 years ago
|
||
This makes things clearer and removes some unnecessary nsresult checks. The patch also: - removes some unnecessary |new| and moz_xmalloc() checks; - adds MOZ_MUST_USE to some fallible nsVariant methods.
Attachment #8746947 -
Flags: review?(erahm)
Assignee | ||
Comment 3•8 years ago
|
||
It's always NS_OK. The patch also removes an unnecessary failure check as a result.
Attachment #8746948 -
Flags: review?(erahm)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8746949 -
Flags: review?(erahm)
Assignee | ||
Updated•8 years ago
|
Attachment #8746948 -
Attachment is obsolete: true
Attachment #8746948 -
Flags: review?(erahm)
Assignee | ||
Comment 5•8 years ago
|
||
A few callers of NS_NewISupportsArray() didn't use the return value to detect failure, but instead checked if the |array| argument was null after the call. This is inconsistent with the majority of the calls to NS_NewISupportsArray(). This patch changes them to be checked in the normal way.
Attachment #8746950 -
Flags: review?(erahm)
Assignee | ||
Comment 6•8 years ago
|
||
As well as adding MOZ_MUST_USE to a number of functions, this patch: - Changes the return type of nsObserverList::GetObserverList() from |nsresult| to |void|, because it always returned NS_OK and none of the callers checked the result. - Removes an unnecessary |new| check in nsSupportsArray::Enumerate().
Attachment #8746951 -
Flags: review?(erahm)
Updated•8 years ago
|
Attachment #8746946 -
Flags: review?(erahm) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8746947 [details] [diff] [review] (part 2) - Make infallible nsVariant methods return |void| instead of |nsresult| Review of attachment 8746947 [details] [diff] [review]: ----------------------------------------------------------------- It's a little weird that some |Set| calls are still fallible, but I guess that's existing behavior. ::: xpcom/ds/nsVariant.cpp @@ +1200,5 @@ > #define DATA_SETTER_PROLOGUE \ > Cleanup() > > #define DATA_SETTER_EPILOGUE(type_) \ > mType = nsIDataType::type_; \ nit: remove trailing slash
Attachment #8746947 -
Flags: review?(erahm) → review+
Updated•8 years ago
|
Attachment #8746949 -
Flags: review?(erahm) → review+
Updated•8 years ago
|
Attachment #8746950 -
Flags: review?(erahm) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8746951 [details] [diff] [review] (part 5) - Use MOZ_MUST_USE in other parts of xpcom/ds/ Review of attachment 8746951 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/ds/Tokenizer.h @@ +299,5 @@ > * > * Calling Rollback() after ReadUntil() will return the read cursor to the > * position it had before ReadUntil was called. > */ > + MOZ_MUST_USE bool nit: Header style is return type on the same line, right? @@ +302,5 @@ > */ > + MOZ_MUST_USE bool > + ReadUntil(Token const& aToken, nsDependentCSubstring& aResult, > + ClaimInclusion aInclude = EXCLUDE_LAST); > + MOZ_MUST_USE bool same ::: xpcom/ds/nsPersistentProperties.h @@ +24,5 @@ > NS_DECL_THREADSAFE_ISUPPORTS > NS_DECL_NSIPROPERTIES > NS_DECL_NSIPERSISTENTPROPERTIES > > + static MOZ_MUST_USE nsresult same ::: xpcom/ds/nsSupportsArray.h @@ +20,5 @@ > > public: > nsSupportsArray(void); > + > + static MOZ_MUST_USE nsresult Same applies the the rest that were modified.
Attachment #8746951 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 9•8 years ago
|
||
> It's a little weird that some |Set| calls are still fallible, but I guess
> that's existing behavior.
Yeah, a few are fallible for one reason or another. The original authors chose to make the return types consistent, but I think that's less important than making infallibility clear, because it avoids the need for useless MOZ_MUST_USE annotations, and also avoids unnecessary checks of infallible functions.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #8) > > nit: Header style is return type on the same line, right? When it all fits on the one line yes. But otherwise, you have to find somewhere to break it. Our style guide isn't clear on this, but the two obvious choices are (a) after the return type, or (b) after an argument. In these cases I chose (a) because I think it looks better. There is plenty of precedent for both choices.
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a2873bb694e50649eb258cfa957a81bd45841f Bug 1268772 (part 1) - Remove nsCheapSet::Put()'s return value. r=erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/4cbd94383852a0f3840d59b951fc6b45f9e64d01 Bug 1268772 (part 2) - Make infallible nsVariant methods return |void| instead of |nsresult|. r=erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/a2061ce934cbd547eb692165d819224a40d940c8 Bug 1268772 (part 3) - Remove NS_NewWindowsRegKey()'s return value. r=erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1b48e0c972772ddad2708febc86f3ab10c5c28 Bug 1268772 (part 4) - Use MOZ_MUST_USE with NS_NewISupportsArray(). r=erahm. https://hg.mozilla.org/integration/mozilla-inbound/rev/1d99e6c5a2bf41b983658369b97c5d152649695b Bug 1268772 (part 5) - Use MOZ_MUST_USE in other parts of xpcom/ds/. r=erahm.
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/18a2873bb694 https://hg.mozilla.org/mozilla-central/rev/4cbd94383852 https://hg.mozilla.org/mozilla-central/rev/a2061ce934cb https://hg.mozilla.org/mozilla-central/rev/ca1b48e0c972 https://hg.mozilla.org/mozilla-central/rev/1d99e6c5a2bf
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Updated•8 years ago
|
Blocks: use-nodiscard
Assignee | ||
Comment 13•8 years ago
|
||
erahm, please review the xpcom/ changes. bagder, please review the netwerk/ change. I wasn't sure if aborting on failure was the best approach here. mattwoodrow, please review the gfx/ change.
Attachment #8754212 -
Flags: review?(matt.woodrow)
Attachment #8754212 -
Flags: review?(erahm)
Attachment #8754212 -
Flags: review?(daniel)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8754212 [details] [diff] [review] (part 1) - Use MOZ_MUST_USE more in xpcom/io/Base64.h Apologies! This patch was supposed to go to bug 1274148.
Attachment #8754212 -
Attachment is obsolete: true
Attachment #8754212 -
Flags: review?(matt.woodrow)
Attachment #8754212 -
Flags: review?(erahm)
Attachment #8754212 -
Flags: review?(daniel)
You need to log in
before you can comment on or make changes to this bug.
Description
•