819700 - Cleanup of self-hosting-related code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Cleanups to be attached in three patches:
1. Moving all self-hosting related C++ code into vm/SelfHosting.cpp
2. Removing the "%funName()" syntax
3. Sanitizing intrinsic functions naming and arguments order

Comment 1

[Till Schneidereit [:till]]

Attachment: Part 3: Cleanup of intrinsics naming and arguments order of callFunction. r=?

Comment 2

[Till Schneidereit [:till]]

Attachment: Part 1: Move all self-hosting code from jscntxt.cpp to vm/SelfHosting.cpp. r=?

Comment 3

[Till Schneidereit [:till]]

Attachment: Part 2: Remove support for the "%FunName" syntax in self-hosted code

Comment 4

[Till Schneidereit [:till]]

The following usage-relevant changes have been made:

%_CallFunction(thisArg, ...args, fun) is now callFunction(fun, thisArg, ...args)

ToObject -> toObject
ToInteger -> toInteger
IsCallable -> isCallable
ThrowError -> throwError
_MakeConstructible -> makeConstructible
_DecompileArg -> decompileArg


Try-servering here:
https://tbpl.mozilla.org/?tree=Try&rev=b131b3eb3e9c

Comment 5

[Jeff Walden [:Waldo]]

Given that the spec ops these methods presumably implement have that capitalization, I think we do want to stick with "ToObject", "ToInteger", and "IsCallable".  The others I don't much care about one way or another.

Comment 6

[Till Schneidereit [:till]]

Hrmpf, that is a pretty good point.

I'm somewhat on the fence about the naming scheme in general: It seems somewhat desirable for the intrinsic functions to stick out, but I can't really give a good reason for that.

Tending towards first letter capitalizing all of them, now.

Comment 7

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

Comment on attachment 690121 [details] [diff] [review]
Part 1: Move all self-hosting code from jscntxt.cpp to vm/SelfHosting.cpp. r=?

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

::: js/src/vm/SelfHosting.cpp
@@ +40,5 @@
> +    RootedValue val(cx, args[0]);
> +    RootedObject obj(cx, ToObject(cx, val));
> +    if (!obj)
> +        return false;
> +    args.rval().set(OBJECT_TO_JSVAL(obj));

This should become

args.rval().setObject(*obj);

at some point.

@@ +51,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    double result;
> +    if (!ToInteger(cx, args[0], &result))
> +        return false;
> +    args.rval().set(DOUBLE_TO_JSVAL(result));

Ditto

@@ +61,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    Value val = args[0];
> +    bool isCallable = val.isObject() && val.toObject().isCallable();
> +    args.rval().set(BOOLEAN_TO_JSVAL(isCallable));

Ditto

@@ +239,5 @@
> +        RootedFunction fun(cx, funVal.toObject().toFunction());
> +        RootedObject clone(cx, CloneFunctionObject(cx, fun, cx->global(), fun->getAllocKind()));
> +        if (!clone)
> +            return false;
> +        vp.set(ObjectValue(*clone));

vp.setObject(*clone);

@@ +241,5 @@
> +        if (!clone)
> +            return false;
> +        vp.set(ObjectValue(*clone));
> +    } else {
> +        vp.set(UndefinedValue());

vp.setUndefined();

Comment 8

[Till Schneidereit [:till]]

(In reply to :Ms2ger from comment #7)

Thanks, all incorporated

Comment 9

[Mozilla RelEng Bot]

Try run for b131b3eb3e9c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b131b3eb3e9c
Results (out of 255 total builds):
    success: 230
    warnings: 25
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b131b3eb3e9c

Comment 10

[Norbert Lindenberg]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Given that the spec ops these methods presumably implement have that
> capitalization, I think we do want to stick with "ToObject", "ToInteger",
> and "IsCallable".  The others I don't much care about one way or another.

(In reply to Till Schneidereit [:till] from comment #6)
> Hrmpf, that is a pretty good point.
> 
> I'm somewhat on the fence about the naming scheme in general: It seems
> somewhat desirable for the intrinsic functions to stick out, but I can't
> really give a good reason for that.
> 
> Tending towards first letter capitalizing all of them, now.

Using the same spelling as in the standards is a good goal, but runs counter to the standard practice of capitalizing constructors, and only those (well, and namespace objects such as Math, Intl, JSON). My build script currently has to map all intrinsic names to lower case before running JSHint on my JS code - it would be nice to get rid of that. In attachment 687536 [details] [diff] [review] I'm using __SpecOps - would that work?

Comment 11

[Till Schneidereit [:till]]

(In reply to Norbert Lindenberg from comment #10)
> > Tending towards first letter capitalizing all of them, now.
> 
> Using the same spelling as in the standards is a good goal, but runs counter
> to the standard practice of capitalizing constructors, and only those (well,
> and namespace objects such as Math, Intl, JSON). My build script currently
> has to map all intrinsic names to lower case before running JSHint on my JS
> code - it would be nice to get rid of that. In attachment 687536 [details] [diff] [review]
> [diff] [review] I'm using __SpecOps - would that work?

As I said in bug 769872 comment 61, I wonder which style we should strive to adhere to: the one established in the JS community, or the one the rest of SpiderMonkey is using.

I don't mean this as a rhetorical question: I'm honestly not sure which of these options makes more sense.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 690122 [details] [diff] [review]
Part 2: Remove support for the "%FunName" syntax in self-hosted code

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

Looks fine, mostly removals.

::: js/src/jsopcode.tbl
@@ +360,4 @@
>   */
>  OPDEF(JSOP_INTRINSICNAME, 143, "intrinsicname", NULL, 5,  0,  1, 19,  JOF_ATOM|JOF_NAME|JOF_TYPESET)
>  OPDEF(JSOP_CALLINTRINSIC, 144, "callintrinsic", NULL, 5,  0,  1, 19,  JOF_ATOM|JOF_NAME|JOF_TYPESET)
>  

Should these opcodes be removed?

Comment 13

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #12)
> Comment on attachment 690122 [details] [diff] [review]
> Part 2: Remove support for the "%FunName" syntax in self-hosted code
> 
> Review of attachment 690122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine, mostly removals.
> 
> ::: js/src/jsopcode.tbl
> @@ +360,4 @@
> >   */
> >  OPDEF(JSOP_INTRINSICNAME, 143, "intrinsicname", NULL, 5,  0,  1, 19,  JOF_ATOM|JOF_NAME|JOF_TYPESET)
> >  OPDEF(JSOP_CALLINTRINSIC, 144, "callintrinsic", NULL, 5,  0,  1, 19,  JOF_ATOM|JOF_NAME|JOF_TYPESET)
> >  
> 
> Should these opcodes be removed?

Scratch this comment, don't know what I was thinking. Looks good overall.

Comment 14

[Till Schneidereit [:till]]

Thanks for the reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e668236e934
https://hg.mozilla.org/integration/mozilla-inbound/rev/36b37b58c0c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d444cab11e7

Comment 15

[Till Schneidereit [:till]]

And
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f49c4f231f

Thanks for noticing, Ms2ger!

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e668236e934
https://hg.mozilla.org/mozilla-central/rev/36b37b58c0c2
https://hg.mozilla.org/mozilla-central/rev/8d444cab11e7
https://hg.mozilla.org/mozilla-central/rev/12f49c4f231f

Comment 17

[Mozilla RelEng Bot]

Try run for b131b3eb3e9c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b131b3eb3e9c
Results (out of 256 total builds):
    success: 230
    warnings: 25
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b131b3eb3e9c