Closed Bug 901750 Opened 6 years ago Closed 6 years ago

Change return type of |JSNative| from |JSBool| to |bool|.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files)

JSNative's return type is a major contributor to JSBool usage.  I intend to
convert it to return bool.
luke: please review the js/ changes.

bz: please review everything else.  The dom/bindings/Codegen.py changes deserve
close scrutiny.  (More about that below.)

Apologies for a tedious review.  If it's any consolation, it was even more
tedious to write :)

----

This patch changes JSNative's return type from JSBool to bool, which affects a
lot of code.  

In JSNative functions, I correspondingly converted a lot of |return
JS_{TRUE,FALSE};| statements to |return {true,false};|.  (I may have also
missed some.)

Frequencies before and after:

  JSBool    3529 --> 2572
  JS_TRUE    400 -->  294
  JS_FALSE   698 -->  421
  ----
  total     4627 --> 3287 (29% fewer)

I also made a handful of other minor changes while in there:

- I made some of these JSNative functions static.
- I made some related funcs return bool instead of JSBool (e.g. helpers called
  by JSNative functions).
- I made some very minor formatting changes in a handful of places.

In Codegen.py there was one tricky change -- I made
CGAbstractBindingMethod.__init__ default to |bool| instead of |JSBool|.  I then
looked at every place it was called, and did the following:

JSNatives (return type is |bool|)
- LEGACYCALLER_HOOK_NAME
- genericMethod
- genericGetter, genericLenientGetter
- genericSetter, genericLenientSetter

Others (return type is |bool|)
- ResolveOwnPropertyViaNewresolve
- EnumerateOwnPropertiesViaGetOwnPropertyNames

Others (return type is |JSBool|)
- NEWRESOLVE_HOOK_NAME
- ENUMERATE_HOOK_NAME

Finally, by itself this patch causes lots of failures.  But before landing I
will combine it with jandem's 2nd patch in bug 899017, which makes things work
ok.  Try runs:
- https://tbpl.mozilla.org/?tree=Try&rev=a3d4274e09d9
- https://tbpl.mozilla.org/?tree=Try&rev=7de63dad9bbe
Attachment #786015 - Flags: review?(luke)
Attachment #786015 - Flags: review?(bzbarsky)
Attachment #786015 - Flags: review?(luke) → review+
Comment on attachment 786015 [details] [diff] [review]
Change return type of |JSNative| from |JSBool| to |bool|.

r=me on the browser js/src/xpconnect parts.

Some things that are worth double-checking:

1)  In js.cpp, I believe we need to change the return types of its_get_customNative, its_set_customNative, dom_genericGetter, dom_genericSetter.

2)  In Debugger.cpp, I believe we need to change the return types of all the things appearing in JS_PSG/JS_PSGS: Debugger::get/setEnabled, Debugger::get/setOnDebuggerStatement, Debugger::get/setOnExceptionUnwind, Debugger::getsetOnNewScript, Debugger::get/setOnEnterFrame, Debugger::get/setOnNewGlobalObject, Debugger::get/setUncaughtExceptionHook, DebuggerScript_getURL, DebuggerScript_getStartLine, DebuggerScript_getLineCount, DebuggerScript_getSource, DebuggerScript_getSourceStart, DebuggerScript_getSourceLength, DebuggerScript_getStaticLevel, DebuggerScript_getSourceMapUrl, DebuggerSource_getText, DebuggerSource_getUrl, etc, etc.

The non-typesafe casting in JS_FS is just terrible.  :(  The only good news is that we have very few callsites that use JSPROP_NATIVE_ACCESSORS, and I think you got all of those except the ones I list above.

r=me with the debugger and js shell bits fixed.
Attachment #786015 - Flags: review?(bzbarsky) → review+
Blocks: 898914
Good point about the JS_PSG stuff.  Here's a patch (just in case you're
curious) with those points fixed.

I looked for any other instances of JS_PSG, JS_PSGS, |(JSPropertyOp)| and
|(JSStrictPropertyOp)|.  I'm pretty sure this patch gets all of them.
Yep, looks good to me.
Comment on attachment 786015 [details] [diff] [review]
Change return type of |JSNative| from |JSBool| to |bool|.

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

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +1463,2 @@
>  
>    return CallNPMethodInternal(cx, obj, argc, JS_ARGV(cx, vp), vp, false);

Maybe change CallNPMethodInternal too.
https://hg.mozilla.org/mozilla-central/rev/c9829bff3ae4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.