Properly create variables to access outer arguments and function names.

Properly create variables to access outer arguments and function names.

Committed: http://code.google.com/p/v8/source/detail?r=6380

[antonm, 2011-01-18 20:42:26]

Guys,

may you have a look?

Alas, I failed to create a repro for function lookup, apparently it goes some
different path.  Still, I am adding handling of this case.

[Kevin Millikin (Chromium), 2011-01-19 07:39:57]

LGTM after addressing comments below.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode161
src/scopes.cc:161: // and thread access to this scope's parameters via arguments
shadow.
Suggested rewording, something like:

This scope's arguments shadow (if present) is context-allocated if an inner
scope accesses this one's parameters.  Allocate the arguments_shadow_ variable
if necessary.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode261
src/scopes.cc:261: // as it is impllicitly present in any scope.
Could you fix the spelling of "implicitly"?

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode269
src/scopes.cc:269: ASSERT(scope_info_->StackSlotIndex(*name) < 0);
This assert could be lifted higher (up to the previous assert), couldn't it? 
The comment should say "assert" not "check".

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode286
src/scopes.cc:286: new Literal(Handle<Object>(Smi::FromInt(index))),
Indentation is a bit off here.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode299
src/scopes.cc:299: variables_.Declare(this, name, mode, true, Variable::NORMAL);
Will this be the right mode?

http://codereview.chromium.org/6266007/diff/2001/test/mjsunit/compiler/regres...
File test/mjsunit/compiler/regress-serialized-slots.js (right):

http://codereview.chromium.org/6266007/diff/2001/test/mjsunit/compiler/regres...
test/mjsunit/compiler/regress-serialized-slots.js:28: function runner(f,
expected) {
Test code is complicated enough that it needs a comment saying what it's
testing.

Think of the person who finds a test failure (wrong result) here.

[antonm, 2011-01-19 08:18:18]

Kevin, thanks a lot for comments!

I've submitted the change already not to miss trunk push.  If you have any
additional concerns, please, let me know and I'll address them with a separate
change.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc
File src/scopes.cc (right):

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode161
src/scopes.cc:161: // and thread access to this scope's parameters via arguments
shadow.
On 2011/01/19 07:39:57, Kevin Millikin wrote:
> Suggested rewording, something like:
> 
> This scope's arguments shadow (if present) is context-allocated if an inner
> scope accesses this one's parameters.  Allocate the arguments_shadow_ variable
> if necessary.

Done.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode261
src/scopes.cc:261: // as it is impllicitly present in any scope.
On 2011/01/19 07:39:57, Kevin Millikin wrote:
> Could you fix the spelling of "implicitly"?

Done.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode269
src/scopes.cc:269: ASSERT(scope_info_->StackSlotIndex(*name) < 0);
On 2011/01/19 07:39:57, Kevin Millikin wrote:
> This assert could be lifted higher (up to the previous assert), couldn't it? 
> The comment should say "assert" not "check".

Done.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode286
src/scopes.cc:286: new Literal(Handle<Object>(Smi::FromInt(index))),
On 2011/01/19 07:39:57, Kevin Millikin wrote:
> Indentation is a bit off here.

Done.

http://codereview.chromium.org/6266007/diff/2001/src/scopes.cc#newcode299
src/scopes.cc:299: variables_.Declare(this, name, mode, true, Variable::NORMAL);
On 2011/01/19 07:39:57, Kevin Millikin wrote:
> Will this be the right mode?

Oops, thanks a lot for spotting this.  Now passing explicit constant.

http://codereview.chromium.org/6266007/diff/2001/test/mjsunit/compiler/regres...
File test/mjsunit/compiler/regress-serialized-slots.js (right):

http://codereview.chromium.org/6266007/diff/2001/test/mjsunit/compiler/regres...
test/mjsunit/compiler/regress-serialized-slots.js:28: function runner(f,
expected) {
On 2011/01/19 07:39:57, Kevin Millikin wrote:
> Test code is complicated enough that it needs a comment saying what it's
> testing.
> 
> Think of the person who finds a test failure (wrong result) here.

Done.