Closed Bug 819700 Opened 8 years ago Closed 8 years ago

Cleanup of self-hosting-related code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: till, Assigned: till)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t])

Attachments

(3 files)

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
Attachment #690121 - Flags: review?(luke)
Attachment #690120 - Flags: review?(shu)
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
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.
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 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();
(In reply to :Ms2ger from comment #7)

Thanks, all incorporated
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/tschneidereit@gmail.com-b131b3eb3e9c
(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?
(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.
Whiteboard: [js:t]
Attachment #690121 - Flags: review?(luke) → review+
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?
Attachment #690122 - Flags: review?(shu) → review+
Attachment #690120 - Flags: review?(shu) → review+
(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.
Assignee: general → tschneidereit
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/tschneidereit@gmail.com-b131b3eb3e9c
You need to log in before you can comment on or make changes to this bug.