The default bug view has changed. See this FAQ.

mostly remove the cancer of JSBool in js.cpp

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #628483 - Flags: review?(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.
Attachment #628483 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 628567 [details] [diff] [review]
address review comments - ready for checkin
Attachment #628483 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 628570 [details] [diff] [review]
fixed commit message
Attachment #628567 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 759976
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee0193bef630
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ee0193bef630
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.