The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
General
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

9.76 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
8.16 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
9.50 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.37 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.60 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
4.98 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
3.38 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.01 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
6.66 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 651347 [details] [diff] [review]
Convert declared types to nsresult
Attachment #651347 - Flags: review?(ehsan)
Created attachment 651349 [details] [diff] [review]
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.
Attachment #651349 - Flags: review?(ehsan)
Created attachment 651352 [details] [diff] [review]
Change named constants to correct types
Attachment #651352 - Flags: review?(ehsan)
Created attachment 651354 [details] [diff] [review]
Don't return nsresult from main() (editor/)
Attachment #651354 - Flags: review?(ehsan)
Created attachment 651356 [details] [diff] [review]
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.
Attachment #651356 - Flags: review?(ehsan)
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.
Attachment #651358 - Flags: review?(ehsan)
Created attachment 651359 [details] [diff] [review]
Cast some nsresult to bool where the function really returns bool
Attachment #651359 - Flags: review?(ehsan)
Created attachment 651360 [details] [diff] [review]
Don't treat PRStatus as nsresult
Attachment #651360 - Flags: review?(ehsan)
Created attachment 651367 [details] [diff] [review]
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.
Attachment #651367 - Flags: review?(ehsan)

Updated

5 years ago
Attachment #651347 - Flags: review?(ehsan) → review+
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...
Attachment #651349 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #651352 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #651354 - Flags: review?(ehsan) → review+
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.
Attachment #651356 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #651358 - Flags: review?(ehsan) → review+
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.
Attachment #651359 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #651360 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #651367 - Flags: review?(ehsan) → review+
(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
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
Flags: in-testsuite-
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.