759902 - mostly remove the cancer of JSBool in js.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[:Benjamin Peterson]

Attachment: 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]]

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]

Attachment: address review comments - ready for checkin

Comment 3

[:Benjamin Peterson]

Attachment: fixed commit message

Comment 4

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0193bef630

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ee0193bef630