Preliminary code for block scopes and block contexts.

Preliminary code for block scopes and block contexts.

BUG=
TEST=

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

[Steven, 2011-08-02 15:45:49]

Hi Kevin,
here's some experimental preliminary code from my trials to add block scoping.
This code should only add block scopes and block contexts without considering
any let declarations. I tried to follow existing code for 'with' and 'catch' and
make the change as small as possible.

Here's some guidance around the CL:
* ParseStatement calls a new function ParseScopedBlock instead of ParseBlock
which adds a new block scope to the scope tree. All other calls of ParseBlock
remain the same.
* In each block scope a single variable '+block' is declared.
* Like (so far) for catch variables the variable allocation should always
context allocate the variable in the block scope.
* The block context has a serialized scope info from the block scope in the
extension slot.

I'm kinda stuck in getting this to work. The code always crashes with
# Fatal error in [...]/v8/src/objects.cc, line 154
# CHECK(holder != __null) failed

Please take a look. Maybe you find something I'm missing.

Thanks!
-- Steven

[Kevin Millikin (Chromium), 2011-08-10 11:19:27]

This looks mostly good.  Address the comments below and make sure to run the
presubmit tests and fix any long lines, then I'll take another quick look.

http://codereview.chromium.org/7549008/diff/2001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/7549008/diff/2001/src/heap.h#newcode489
src/heap.h:489: // Allocates a serialized scope info .
Extra space in this comment before the period.

http://codereview.chromium.org/7549008/diff/2001/src/heap.h#newcode677
src/heap.h:677: // Allocate a 'block' context.
No real need for quotes around block.  I guess it's supposed to indicate a JS
keyword in 'with' above.

http://codereview.chromium.org/7549008/diff/2001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7549008/diff/2001/src/parser.cc#newcode1514
src/parser.cc:1514: if (!FLAG_harmony_block_scoping) return ParseBlock(labels,
ok);
This is OK for now, but you should try to think of a way to ultimately turn this
code into:

if (FLAG_harmony_block_scoping) {
  // Preparation.
}
Block* body = ParseBlock(labels, ok);
if (FLAG_harmony_block_scoping) {
  // Cleanup.
}

With the idea of just inlining the code for ParseBlock (provided this is the
only call site) and getting rid of that function.  I can see that's hard to do
right now because of the scoped Target, but once we stop using try/finally here,
I think you could do it.

http://codereview.chromium.org/7549008/diff/2001/src/parser.cc#newcode5154
src/parser.cc:5154: FLAG_harmony_block_scoping = !info->is_native() &&
FLAG_harmony_block_scoping;
I'm always nervous about assigning to the flags.  Since they're global, this
prevents other unrelated code from being able to tell the original value of the
flag.

Can you make this a member variable of the parser, and avoid mutating the flag?

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

http://codereview.chromium.org/7549008/diff/2001/src/scopes.cc#newcode776
src/scopes.cc:776: is_block_scope() || is_catch_scope() ? inner_lookup : true,
This is subtle.  I'd probably change all the identifier names to make it more
clear that "inner_lookup" means a lookup from an inner function scope, something
like from_inner_function.

Then here I'd probably write

var = outer_scope_->LookupRecursive(name,
                                    is_function_scope() || from_inner_function,
                                    invalidated_local);

And later:

var->MarkAsAccessedFromInnerFunctionScope()

etc.

http://codereview.chromium.org/7549008/diff/2001/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7549008/diff/2001/src/scopes.h#newcode432
src/scopes.h:432: // Construct a function scope based on the scope info.
Change the context to say "function or block scope".

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/block-s...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/block-s...
test/mjsunit/harmony/block-scoping.js:1: // Flags: --allow-natives-syntax
--harmony-block-scoping
The test file needs a copyright notice.

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/debug-b...
File test/mjsunit/harmony/debug-blockscopes.js (right):

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/debug-b...
test/mjsunit/harmony/debug-blockscopes.js:1: // Copyright 2008 the V8 project
authors. All rights reserved.
Update the copyright date to 2011.

[Steven, 2011-08-10 12:21:00]

http://codereview.chromium.org/7549008/diff/2001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/7549008/diff/2001/src/heap.h#newcode489
src/heap.h:489: // Allocates a serialized scope info .
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> Extra space in this comment before the period.

Done.

http://codereview.chromium.org/7549008/diff/2001/src/heap.h#newcode677
src/heap.h:677: // Allocate a 'block' context.
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> No real need for quotes around block.  I guess it's supposed to indicate a JS
> keyword in 'with' above.

Done.

http://codereview.chromium.org/7549008/diff/2001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7549008/diff/2001/src/parser.cc#newcode1514
src/parser.cc:1514: if (!FLAG_harmony_block_scoping) return ParseBlock(labels,
ok);
Yes sure. But indeed I think this should be done when getting rid of
try-finally.

I changed the definition of ParseBlock/ParseScopedBlock slightly to use
ParseScopedBlock at all call sites.

On 2011/08/10 11:19:27, Kevin Millikin wrote:
> This is OK for now, but you should try to think of a way to ultimately turn
this
> code into:
> 
> if (FLAG_harmony_block_scoping) {
>   // Preparation.
> }
> Block* body = ParseBlock(labels, ok);
> if (FLAG_harmony_block_scoping) {
>   // Cleanup.
> }
> 
> With the idea of just inlining the code for ParseBlock (provided this is the
> only call site) and getting rid of that function.  I can see that's hard to do
> right now because of the scoped Target, but once we stop using try/finally
here,
> I think you could do it.

http://codereview.chromium.org/7549008/diff/2001/src/parser.cc#newcode5154
src/parser.cc:5154: FLAG_harmony_block_scoping = !info->is_native() &&
FLAG_harmony_block_scoping;
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> I'm always nervous about assigning to the flags.  Since they're global, this
> prevents other unrelated code from being able to tell the original value of
the
> flag.
> 
> Can you make this a member variable of the parser, and avoid mutating the
flag?

Done.

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

http://codereview.chromium.org/7549008/diff/2001/src/scopes.cc#newcode776
src/scopes.cc:776: is_block_scope() || is_catch_scope() ? inner_lookup : true,
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> This is subtle.  I'd probably change all the identifier names to make it more
> clear that "inner_lookup" means a lookup from an inner function scope,
something
> like from_inner_function.
> 
> Then here I'd probably write
> 
> var = outer_scope_->LookupRecursive(name,
>                                     is_function_scope() ||
from_inner_function,
>                                     invalidated_local);
> 
> And later:
> 
> var->MarkAsAccessedFromInnerFunctionScope()
> 
> etc.

Done.

http://codereview.chromium.org/7549008/diff/2001/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/7549008/diff/2001/src/scopes.h#newcode432
src/scopes.h:432: // Construct a function scope based on the scope info.
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> Change the context to say "function or block scope".

Done.

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/block-s...
File test/mjsunit/harmony/block-scoping.js (right):

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/block-s...
test/mjsunit/harmony/block-scoping.js:1: // Flags: --allow-natives-syntax
--harmony-block-scoping
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> The test file needs a copyright notice.

Done.

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/debug-b...
File test/mjsunit/harmony/debug-blockscopes.js (right):

http://codereview.chromium.org/7549008/diff/2001/test/mjsunit/harmony/debug-b...
test/mjsunit/harmony/debug-blockscopes.js:1: // Copyright 2008 the V8 project
authors. All rights reserved.
On 2011/08/10 11:19:27, Kevin Millikin wrote:
> Update the copyright date to 2011.

Done.

[Kevin Millikin (Chromium), 2011-08-11 15:37:41]

LGTM.