Introduce scopes to keep track of catch blocks at compile time.

Introduce scopes to keep track of catch blocks at compile time.

The catch variable is bound in the catch scope.  For simplicity in this
initial implementation, it is always allocated even if unused and always
allocated to a catch context even if it doesn't escape.  The presence of
catch is no longer treated as a with.

In this change, care must be taken to distinguish between the scope where a
var declaration is hoisted to and the scope where the initialization occurs.

R=ager@chromium.org
BUG=
TEST=

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

[Kevin Millikin (Chromium), 2011-06-30 09:28:45]

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc#newcode415
src/parser.cc:415: Scope* Parser::DeclarationScope() {
As an alternative, we could keep a pair of scopes.  I did this for now.

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc#newcode1521
src/parser.cc:1521: Handle<String>* name,
In most cases (all but 'for (var ... in ...)' the variable proxy in the out
parameter was ignored.  Changed to return the name and create the variable proxy
in the caller.

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc#newcode1623
src/parser.cc:1623: Scope* initialization_scope = is_const ? declaration_scope :
top_scope_;
It's subtle that the declaration and initialization are in different scopes.

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc#newcode1940
src/parser.cc:1940: DeclarationScope()->RecordWithStatement();
This is wrong, let's talk about what to do here.

http://codereview.chromium.org/7280012/diff/1001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/runtime.cc#newcode10314
src/runtime.cc:10314: // Finally copy any properties from the function context
extension. This will
Long line already fixed.

[Mads Ager (chromium), 2011-06-30 12:08:32]

LGTM

http://codereview.chromium.org/7280012/diff/1001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:153: int receiver_offset =
info->scope()->num_parameters() * kPointerSize;
I'm getting a little confused here. You don't need to perform this change or any
of the ones below, do you? (Except for the last change that changes a
conditional.) Seems like you should just keep on using the scope() accessor on
the full code generator or get rid of the accessor?

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc#newcode404
src/full-codegen.cc:404: offset += (info_->scope()->num_parameters() + 1) *
kPointerSize;
Do you need this change? Don't you have a scope here already so you can just use
the scope() accessor?

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc#newcode1117
src/full-codegen.cc:1117: Scope* saved_scope = scope();
Could use a comment? We could create a scope object for this if we need it in
more places (but it doesn't look like it is?)

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/parser.cc#newcode1940
src/parser.cc:1940: DeclarationScope()->RecordWithStatement();
On 2011/06/30 09:28:45, Kevin Millikin wrote:
> This is wrong, let's talk about what to do here.

But it is safe, right?

http://codereview.chromium.org/7280012/diff/1001/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

http://codereview.chromium.org/7280012/diff/1001/test/mjsunit/mjsunit.status#...
test/mjsunit/mjsunit.status:33:
##############################################################################
We should file a bug report on this issue.

[Kevin Millikin (Chromium), 2011-06-30 12:30:06]

http://codereview.chromium.org/7280012/diff/1001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:153: int receiver_offset =
info->scope()->num_parameters() * kPointerSize;
I'm being overly defensive.  We are definitely not inside a catch, so scope() ==
info->scope(), but I wanted to insulate against code moving around or cut and
paste.

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc#newcode404
src/full-codegen.cc:404: offset += (info_->scope()->num_parameters() + 1) *
kPointerSize;
This definitely needs to be the function's scope (info_->scope()), not the
current scope which might be a catch scope, where num_parameters() == 0.

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc#newcode1117
src/full-codegen.cc:1117: Scope* saved_scope = scope();
It's only used here and I couldn't think of a comment that was more obvious than
the code itself.  Suggestions?

[Mads Ager (chromium), 2011-06-30 12:47:44]

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc#newcode404
src/full-codegen.cc:404: offset += (info_->scope()->num_parameters() + 1) *
kPointerSize;
On 2011/06/30 12:30:06, Kevin Millikin wrote:
> This definitely needs to be the function's scope (info_->scope()), not the
> current scope which might be a catch scope, where num_parameters() == 0.

Should we create an accessor for that then: function_scope and scope?

http://codereview.chromium.org/7280012/diff/1001/src/full-codegen.cc#newcode1117
src/full-codegen.cc:1117: Scope* saved_scope = scope();
On 2011/06/30 12:30:06, Kevin Millikin wrote:
> It's only used here and I couldn't think of a comment that was more obvious
than
> the code itself.  Suggestions?

Not sure anything is really needed. You are right that the code is pretty self
explanatory. I was thinking something like:

"Use the scope extended with the catch variable for compiling the catch block."

to match the "Extend" comment above which is nice, but doesn't say anything more
than the code either.