Closed Bug 782252 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(9 files)

No description provided.
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)
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)
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)
These methods only return proper error codes, not bools masquerading as nsresult or anything, so this should be safe.
Attachment #651367 - Flags: review?(ehsan)
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+
Attachment #651352 - Flags: review?(ehsan) → review+
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+
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+
Attachment #651360 - Flags: review?(ehsan) → review+
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
You need to log in before you can comment on or make changes to this bug.