Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Description • 9 years ago

Originally I made ModuleEnvironementObject derive from CallObject because it shares many features with it.  However this meant that every ModuleEnvironementObject was also a CallObject according to is(), which was confusing and needed an extra check everywhere we tested whether something was a call scope.  There were probably many places this didn't happen but should have.

Instead we can make a common base class that both derive from which I think makes more sense.

I've called this 'LexicalScopeBase', but that's probably too broad - better suggestions welcome.

	Shu-yu Guo [:shu]
	Comment 1 • 9 years ago

Comment on attachment 8678224 [details] [diff] [review]
disentangle-scope-object

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

I don't see ModuleEnvironmentObject being made a subclass of LexicalScopeBase.

If no tests failed -- that worries me a bit. I'd like to see a 2nd version of the patch because of that + the DebuggerScope thing below.

::: js/src/vm/ScopeObject-inl.h
@@ +48,5 @@
>  }
>  
>  inline void
> +LexicalScopeBase::setAliasedVar(JSContext* cx, AliasedFormalIter fi, PropertyName* name,
> +                              const Value& v)

nit: fix indentation

@@ +58,5 @@
>  }
>  
>  inline void
> +LexicalScopeBase::setAliasedVarFromArguments(JSContext* cx, const Value& argsValue, jsid id,
> +                                           const Value& v)

nit: fix indentation

::: js/src/vm/ScopeObject.cpp
@@ +1620,5 @@
>              return true;
>          }
>  
>          /* Handle unaliased formals, vars, lets, and consts at function scope. */
> +        if (scope->is<LexicalScopeBase>() && !scope->as<CallObject>().isForEval()) {

This check is inconsistent and would fail when scope is a ModuleEnvironmentObject. Moreover the body of the conditional explicitly expects a CallObject. Should this entire if be generalized to LexicalScopeBase?

::: js/src/vm/ScopeObject.h
@@ +295,5 @@
> +    inline void initRemainingSlotsToUninitializedLexicals(uint32_t begin);
> +    inline void initAliasedLexicalsToThrowOnTouch(JSScript* script);
> +
> +  public:
> +    /* Get/set the aliased variable referred to by 'bi'. */

Existing: the parameter is named 'fi'

	Jon Coppeard (:jonco) (PTO until 14th September) Assignee
	Comment 2 • 9 years ago

(In reply to Shu-yu Guo [:shu] from comment #1)
> I don't see ModuleEnvironmentObject being made a subclass of
> LexicalScopeBase.

Ugh, I left out the most import part of the patch.  Fixing this didn't cause any additional problems however.

> ::: js/src/vm/ScopeObject.cpp
> >          /* Handle unaliased formals, vars, lets, and consts at function scope. */
> > +        if (scope->is<LexicalScopeBase>() && !scope->as<CallObject>().isForEval()) {
> 
> This check is inconsistent and would fail when scope is a
> ModuleEnvironmentObject. 

Ah, yes.  At the moment we alias everything in a module scope though so I just changed this back to check for CallObject as it did previously.

	Pulsebot
	Comment 3 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/58d4fc528b3b

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 4 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/58d4fc528b3b