Last Comment Bug 759902 - mostly remove the cancer of JSBool in js.cpp
: mostly remove the cancer of JSBool in js.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla15
Assigned To: :Benjamin Peterson
:
Mentors:
Depends on:
Blocks: 759976
  Show dependency treegraph
 
Reported: 2012-05-30 14:23 PDT by :Benjamin Peterson
Modified: 2012-06-01 08:12 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (66.81 KB, patch)
2012-05-30 14:23 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Review
address review comments - ready for checkin (65.42 KB, patch)
2012-05-30 19:27 PDT, :Benjamin Peterson
no flags Details | Diff | Review
fixed commit message (65.43 KB, patch)
2012-05-30 19:31 PDT, :Benjamin Peterson
no flags Details | Diff | Review

Description :Benjamin Peterson 2012-05-30 14:23:54 PDT
Created attachment 628483 [details] [diff] [review]
patch

JSBool had to stay around the API boundary. (I assume using "true" and "false" with a JSBool type is acceptable.) While I was at it, I made some shell functions static.
Comment 1 Jason Orendorff [:jorendorff] 2012-05-30 19:00:04 PDT
Comment on attachment 628483 [details] [diff] [review]
patch

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

r=me with a few tweaks. (A few Windows-only warnings are not the end of the world; they'll get fixed. But please take a moment to fix the ones you can see.)

::: js/src/shell/js.cpp
@@ +1337,5 @@
>       * unless debug mode is already set to what you're trying to set it to.
>       * In the future, this restriction may be lifted.
>       */
>  
> +    bool ok = JS_SetDebugMode(cx, JSVAL_TO_BOOLEAN(argv[0]));

Assigning a JSBool to a bool causes a warning on MSVC. So in cases like this please use the clown-eyes operator:

  bool ok = !!JS_SetDebugMode(cx, JSVAL_TO_BOOLEAN(argv[0]));

@@ +2285,1 @@
>      ok = JS_ConvertArguments(cx, argc, JS_ARGV(cx, vp), "b/ciujdISWofvZZ*",

Need !! here too.

@@ +2418,3 @@
>      *vp = OBJECT_TO_JSVAL(aobj);
>  
>      ok = JS_GetPropertyDescArray(cx, vobj, &pda);

And here.

@@ +2429,5 @@
>          }
>  
>          /* Protect pdobj from GC by setting it as an element of aobj now */
>          v = OBJECT_TO_JSVAL(pdobj);
>          ok = JS_SetElement(cx, aobj, i, &v);

And here.

@@ +2709,3 @@
>      }
>  
> +    bool saved = false;;

While you're changing this line, please delete one of those semicolons.

@@ +2721,4 @@
>                                            fp->script()->filename,
>                                            JS_PCToLineNumber(cx, fp->script(),
>                                                              fi.pc()),
>                                            vp);

Re-indent the continuation lines, please, since the left-parenthesis on the first line moved.

@@ +4000,2 @@
>      else if (!strcmp(idString.getBytes(), "enum_fail"))
> +        its_enum_fail = js_ValueToBoolean(*vp);

Please revert this change. The purpose of this particular function (if any) is to serve as JSAPI example code, so it should stick to the public API functions.

@@ +4508,2 @@
>          } else {
>              ok = JS_DefineProperty(cx, obj, name, STRING_TO_JSVAL(valstr),

!! here too. Or I suppose you could rewrite the whole if-else block like this:

    ok = valstr && JS_DefineProperty(...);

In either case, remember to re-indent the following line.

@@ +4671,1 @@
>  BindScriptArgs(JSContext *cx, JSObject *obj_, OptionParser *op)

Revert this change!

@@ +4907,5 @@
>      JNIEnv *java_env;
>      JSDJContext *jsdjc;
>  #endif
>  #ifdef JSDEBUGGER_C_UI
> +    bool jsdbc;

JSDEBUGGER_C_UI? Gosh. We should just rip this stuff out. I don't know when it was last compiled by anyone anywhere. Separate bug, I guess.
Comment 2 :Benjamin Peterson 2012-05-30 19:27:51 PDT
Created attachment 628567 [details] [diff] [review]
address review comments - ready for checkin
Comment 3 :Benjamin Peterson 2012-05-30 19:31:12 PDT
Created attachment 628570 [details] [diff] [review]
fixed commit message
Comment 4 Jason Orendorff [:jorendorff] 2012-05-31 14:46:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0193bef630
Comment 5 Ed Morley [:emorley] 2012-06-01 08:12:01 PDT
https://hg.mozilla.org/mozilla-central/rev/ee0193bef630

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