Last Comment Bug 782252 - Fix still more abuse of nsresult (tree-wide, editor/)
: Fix still more abuse of nsresult (tree-wide, editor/)
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Aryeh Gregor (:ayg) (away until October 25)
:
Mentors:
Depends on:
Blocks: nsresult-enum-class
  Show dependency treegraph
 
Reported: 2012-08-13 06:16 PDT by Aryeh Gregor (:ayg) (away until October 25)
Modified: 2012-08-15 18:46 PDT (History)
1 user (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Convert declared types to nsresult (9.76 KB, patch)
2012-08-13 06:18 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Cast NS_ENUMERATOR_FALSE to nsresult (8.16 KB, patch)
2012-08-13 06:21 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Change named constants to correct types (9.50 KB, patch)
2012-08-13 06:25 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Don't return nsresult from main() (editor/) (1.37 KB, patch)
2012-08-13 06:25 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Cast away more setjmp()/longjmp() nsresult errors (3.60 KB, patch)
2012-08-13 06:29 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Cast some nsresult to PRUint32 (4.98 KB, patch)
2012-08-13 06:32 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Cast some nsresult to bool where the function really returns bool (3.38 KB, patch)
2012-08-13 06:32 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Don't treat PRStatus as nsresult (1.01 KB, patch)
2012-08-13 06:33 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review
Use NS_FAILED instead of boolean test for ToInteger()/ToFloat() (6.66 KB, patch)
2012-08-13 06:50 PDT, Aryeh Gregor (:ayg) (away until October 25)
ehsan: review+
Details | Diff | Splinter Review

Description Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:16:53 PDT

    
Comment 1 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:18:17 PDT
Created attachment 651347 [details] [diff] [review]
Convert declared types to nsresult
Comment 2 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:21:12 PDT
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.
Comment 3 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:25:03 PDT
Created attachment 651352 [details] [diff] [review]
Change named constants to correct types
Comment 4 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:25:52 PDT
Created attachment 651354 [details] [diff] [review]
Don't return nsresult from main() (editor/)
Comment 5 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:29:35 PDT
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.
Comment 6 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:32:00 PDT
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.
Comment 7 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:32:50 PDT
Created attachment 651359 [details] [diff] [review]
Cast some nsresult to bool where the function really returns bool
Comment 8 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:33:42 PDT
Created attachment 651360 [details] [diff] [review]
Don't treat PRStatus as nsresult
Comment 9 Aryeh Gregor (:ayg) (away until October 25) 2012-08-13 06:50:45 PDT
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.
Comment 10 :Ehsan Akhgari 2012-08-13 09:55:21 PDT
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 :Ehsan Akhgari 2012-08-13 10:04:25 PDT
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 :Ehsan Akhgari 2012-08-13 10:07:30 PDT
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) (away until October 25) 2012-08-14 02:52:19 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.