Closed Bug 828020 Opened 7 years ago Closed 7 years ago

exactly root StaticScopeIter

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file)

Attached patch v1Splinter Review
Trying this for the first time. Passes jit-tests without JS_GC_ZEAL=6.
Attachment #699418 - Flags: review?(terrence)
Assignee: general → evilpies
Blocks: ExactRooting
Status: NEW → ASSIGNED
Attachment #699418 - Attachment is patch: true
Comment on attachment 699418 [details] [diff] [review]
v1

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

Wow, that was fast and it looks pretty much perfect: r+ with the nits fixed.

::: js/src/vm/ScopeObject.cpp
@@ +22,5 @@
>  using namespace js::types;
>  
>  /*****************************************************************************/
>  
> +StaticScopeIter::StaticScopeIter(JSContext *cx, HandleObject obj_)

We've been moving to postfixing with Arg rather than _, so objArg rather than obj_. I would tend to favor using mozilla's standard 'a' prefix, but I lost that vote.

@@ +23,5 @@
>  
>  /*****************************************************************************/
>  
> +StaticScopeIter::StaticScopeIter(JSContext *cx, HandleObject obj_)
> +  : obj(cx, NULL), onNamedLambda(false)

A nice feature of Rooted is that it implicitly starts as NULL, saving us some code and the worry of following an uninitialized pointer. However, you have the target value available in the constructor, so just make this obj(cx, objArg) and remove the explicit assignment in the constructor.

@@ +32,5 @@
>  
>  bool
>  StaticScopeIter::done() const
>  {
> +    return !obj;

Nice!

@@ +99,5 @@
>      JS_ASSERT(pc >= script->code && pc < script->code + script->length);
>      JS_ASSERT(JOF_OPTYPE(*pc) == JOF_SCOPECOORD);
>  
>      uint32_t blockIndex = GET_UINT32_INDEX(pc + 2 * sizeof(uint16_t));
> +    RootedObject innermostStaticScope(cx, NULL);

I prefer to leave out the NULL.

::: js/src/vm/ScopeObject.h
@@ +100,5 @@
>   * Return a scope iterator pointing at the static scope containing the variable
>   * accessed by the ALIASEDVAR op at 'pc'.
>   */
> +extern UnrootedShape
> +ScopeCoordinateToStaticScopeShape(JSContext *cx, JSScript *script, jsbytecode *pc);

A bit of grepping seems to show that this replaces all of the uses of scopeShape. Since scopeShape is a 3 liner, it would probably be clearer to just inline it and remove scopeShape from the class.
Attachment #699418 - Flags: review?(terrence) → review+
Thanks for the review.

> ::: js/src/vm/ScopeObject.cpp
> @@ +22,5 @@
> >  using namespace js::types;
> >  
> >  /*****************************************************************************/
> >  
> > +StaticScopeIter::StaticScopeIter(JSContext *cx, HandleObject obj_)
> 
> We've been moving to postfixing with Arg rather than _, so objArg rather
> than obj_. I would tend to favor using mozilla's standard 'a' prefix, but I
> lost that vote.
> 
Oh I made this change before I figured out what actually prevented it from compiling. Thanks to clang!

> ::: js/src/vm/ScopeObject.h
> @@ +100,5 @@
> >   * Return a scope iterator pointing at the static scope containing the variable
> >   * accessed by the ALIASEDVAR op at 'pc'.
> >   */
> > +extern UnrootedShape
> > +ScopeCoordinateToStaticScopeShape(JSContext *cx, JSScript *script, jsbytecode *pc);
> 
> A bit of grepping seems to show that this replaces all of the uses of
> scopeShape. Since scopeShape is a 3 liner, it would probably be clearer to
> just inline it and remove scopeShape from the class.

I did this, but didn't really like the look of it.
(In reply to Tom Schuster [:evilpie] from comment #2)
> Thanks for the review.

Thanks for the help with exact rooting!
 
> > A bit of grepping seems to show that this replaces all of the uses of
> > scopeShape. Since scopeShape is a 3 liner, it would probably be clearer to
> > just inline it and remove scopeShape from the class.
> 
> I did this, but didn't really like the look of it.

Okay, good to know! Go ahead and check it in then.
https://hg.mozilla.org/mozilla-central/rev/9a93bc7b059b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.