478075 - js_GetCallObject doesn't need 'parent' argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

jsfun.cpp:js_GetCallObject takes a 'parent' argument, provides a default  value if it's NULL, but then asserts that the parent must be the scopeChain of the passed-in fp.

It's surprising for a function to take an argument, suggesting that its behavior can meaningfully vary depending on the value passed in, and then discover that there's only one permissible value --- which some callers compute in a circuitous way. 

We should just drop the 'parent' argument, and always set the parent from the fp.

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 478075: Remove parent argument to js_GetCallObject.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 362774 [details] [diff] [review]
Bug 478075: Remove parent argument to js_GetCallObject.

>+    /* Create the call object, using the frame's enclosing scope as
>+       its parent, and link the call to its stack frame. */

Nit: looks good but this "Harbison&Steele" style is not consistent with prevailing major comment style.

Anything to assert about fp->scopeChain here? I'm not sure, but the thought occurs.

>+    /* Push callobj on the top of the scope chain, and make it the
>+       variables object. */

Another good-lookin' but non-conformant multiline comment. If enough people prefer this style and don't mind two multiline comment styles, let's add it to the style guide. We should have a straw IRC poll or something.

Thanks,

/be

Comment 3

[Jim Blandy :jimb]

Attachment: Bug 478075: Remove parent argument to js_GetCallObject.

Revised comments; added assert.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 362974 [details] [diff] [review]
Bug 478075: Remove parent argument to js_GetCallObject.

>+#ifdef DEBUG
>+    /* A call object should be a frame's outermost scope chain element.  */
>+    JSClass *classp = OBJ_GET_CLASS(cx, fp->scopeChain);
>+    if (classp == &js_WithClass
>+        || classp == &js_BlockClass
>+        || classp == &js_CallClass)

Thanks, good assert. You could use JS_ASSERT_IF(x,y) instead of if (x) JS_ASSERT(y).

More nits: && and || at end of line, other ops generally at start of continuation (it is what it is, to some extent; mainly aligning conjuctive and alternative terms' starting columns seems a win, whereas it's easy to miss single-char operators at the end, and who said the trailing terms were created equal :-P -- but prevailing style is when-in-Rome for code changes, until we build new Rome or it gets sacked by vandals ;-).

The other prevailing style thing would be if any condition were multiline (or then, or else -- even if only due to a comment), then brace everything.

Also why not use our new tw=99 order and put the condition on one line? Thanks, r=me with the one-line condition nit-pick.

/be

Comment 5

[Jim Blandy :jimb]

Thanks!
Landed in TM: http://hg.mozilla.org/tracemonkey/rev/b9da8c3fd3ad

Comment 6

[Nochum Sossonko [:Natch]]

m-c: http://hg.mozilla.org/mozilla-central/rev/b9da8c3fd3ad , ftw!

Comment 7

[Nochum Sossonko [:Natch]]

m-191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/176061aa5ccf