782252 - Fix still more abuse of nsresult (tree-wide, editor/)

Status: RESOLVED FIXED

(Core :: General, enhancement)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Convert declared types to nsresult

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Cast NS_ENUMERATOR_FALSE to nsresult

This is not very pretty, to put it mildly, but I don't want to make NS_ENUMERATOR_FALSE an actual nsresult because it's a horrible abuse.  I also don't want to convert these to "enumerator->IsDone() != NS_OK", because that's a lot more confusing to read.

Perhaps we could make IsDone notxpcom, change the return type to bool, invert the meaning, fix all callers in the tree, and add a new IsDone for XPCOM callers that's a no-op, with a different binaryname?  I could do that instead of this patch.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Change named constants to correct types

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Don't return nsresult from main() (editor/)

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Cast away more setjmp()/longjmp() nsresult errors

I actually think the int argument passed to setjmp/longjmp might be arbitrary, in which case this code is really correct after all.  That is, I think the second argument to longjmp is just the value that setjmp will return.  So it's not technically incorrect as long as all callers are treating the value consistently.  So maybe the nasty comments are unwarranted.  But we need the casts.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Cast some nsresult to PRUint32

All seemingly legitimate casts: printing, keys in a map.  I could overload AppendInt so the casts for it are unnecessary, but it doesn't seem particularly necessary.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Cast some nsresult to bool where the function really returns bool

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Don't treat PRStatus as nsresult

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Use NS_FAILED instead of boolean test for ToInteger()/ToFloat()

These methods only return proper error codes, not bools masquerading as nsresult or anything, so this should be safe.

Comment 10

[(no longer active)]

Comment on attachment 651349 [details] [diff] [review]
Cast NS_ENUMERATOR_FALSE to nsresult

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

Sigh... this is fine for now, but the better fix would be to move away toward using nsISimpleEnumerator where possible...

Comment 11

[(no longer active)]

Comment on attachment 651356 [details] [diff] [review]
Cast away more setjmp()/longjmp() nsresult errors

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

(In reply to :Aryeh Gregor from comment #5)
> I actually think the int argument passed to setjmp/longjmp might be
> arbitrary, in which case this code is really correct after all.  That is, I
> think the second argument to longjmp is just the value that setjmp will
> return.  So it's not technically incorrect as long as all callers are
> treating the value consistently.  So maybe the nasty comments are
> unwarranted.  But we need the casts.

Yeah, setjmp returns the second argument passed to longjmp, so I think those comments should be removed.

Comment 12

[(no longer active)]

Comment on attachment 651359 [details] [diff] [review]
Cast some nsresult to bool where the function really returns bool

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

::: netwerk/base/src/nsLoadGroup.cpp
@@ +722,5 @@
>  {
>      RequestMapEntry *e = static_cast<RequestMapEntry *>(hdr);
>      nsISupportsArray *array = static_cast<nsISupportsArray *>(arg);
>  
> +    // nsSupportsArray::AppendElement returns a bool disguised as nsresult

Nit: nsISupportsArray.

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Aryeh Gregor from comment #6)
> Created attachment 651358 [details] [diff] [review]
> Cast some nsresult to PRUint32
> 
> All seemingly legitimate casts: printing, keys in a map.  I could overload
> AppendInt so the casts for it are unnecessary, but it doesn't seem
> particularly necessary.

I had to drop the ipc/chromium/src/chrome/common/ipc_message_utils.h part of this patch for now, because it depends on the change to enum, which hasn't happened yet.  I'll fold it into the patch that switches over to enum class.

I'll land this when it passes try: https://tbpl.mozilla.org/?tree=Try&rev=14dafce4ed02

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec6f3cb2358
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5f3b3cc280
https://hg.mozilla.org/integration/mozilla-inbound/rev/e158214617dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/494157fa4561
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1972ad497b
https://hg.mozilla.org/integration/mozilla-inbound/rev/239d64d31b1b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d6abce30357
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8476ef1d352
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cad366318ba

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ec6f3cb2358
https://hg.mozilla.org/mozilla-central/rev/fb5f3b3cc280
https://hg.mozilla.org/mozilla-central/rev/e158214617dd
https://hg.mozilla.org/mozilla-central/rev/494157fa4561
https://hg.mozilla.org/mozilla-central/rev/fc1972ad497b
https://hg.mozilla.org/mozilla-central/rev/239d64d31b1b
https://hg.mozilla.org/mozilla-central/rev/5d6abce30357
https://hg.mozilla.org/mozilla-central/rev/f8476ef1d352
https://hg.mozilla.org/mozilla-central/rev/2cad366318ba