Closed Bug 1268772 Opened 8 years ago Closed 8 years ago

Use MOZ_MUST_USE more in xpcom/ds/

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(5 files, 2 obsolete files)

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.
It's always NS_OK and none of the call sites look at it.
Attachment #8746946 - Flags: review?(erahm)
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)
It's always NS_OK. The patch also removes an unnecessary failure check as a
result.
Attachment #8746948 - Flags: review?(erahm)
Attachment #8746948 - Attachment is obsolete: true
Attachment #8746948 - Flags: review?(erahm)
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)
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)
Attachment #8746946 - Flags: review?(erahm) → review+
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+
Attachment #8746949 - Flags: review?(erahm) → review+
Attachment #8746950 - Flags: review?(erahm) → review+
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+
> 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.
(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.
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.
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)
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.

Attachment

General

Created:
Updated:
Size: