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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

JSNative's return type is a major contributor to JSBool usage.  I intend to
convert it to return bool.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Change return type of |JSNative| from |JSBool| to |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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Change return type of |JSNative| from |JSBool| to |bool|.

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yep, looks good to me.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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