[webkit-reviews] review granted: [Bug 161099] We initialize functions too early in an eval : [Attachment 308110] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted GSkachkov <gskachkov at gmail.com>'s
request for review:
Bug 161099: We initialize functions too early in an eval
https://bugs.webkit.org/show_bug.cgi?id=161099

Attachment 308110: Patch

https://bugs.webkit.org/attachment.cgi?id=308110&action=review




--- Comment #13 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 308110
  --> https://bugs.webkit.org/attachment.cgi?id=308110
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308110&action=review

r=me

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:129
> +		       RefPtr<RegisterID> tolLevelObjectScope =
emitResolveScope(nullptr, Variable(metadata->ident()));
> +		       tolLevelScope = newBlockScopeVariable();
> +		       emitMove(tolLevelScope.get(),
tolLevelObjectScope.get());

I wonder if the solution in the future is to use your new scope resolution
bytecode. Since a syntax error is created if it returns undefined, I think
we're guaranteed to find the correct scope.
That said, we'd do it only for sloppy mode, and for strict mode, we would still
do this code that's written here. Maybe you'll need to move to this when doing
the catch variable.

Also, I think at some point, we should move strict mode off of using the
StrictActivation object. It should just use a locally created lexical
environment.

> JSTests/stress/eval-func-decl-with-let-const-class.js:78
> +	   let f;

can you assert the value of "f" here after the evals.

> JSTests/stress/eval-func-decl-with-let-const-class.js:87
> +	   let f;

ditto

> JSTests/stress/eval-func-decl-with-let-const-class.js:95
> +	   let f;

ditto

> JSTests/stress/eval-func-decl-with-let-const-class.js:102
> +	   let f;

ditto