Add support for all declarations in the top-level compiler: ...

Add support for all declarations in the top-level compiler: 
 
Until now we only handled global declarations. This change
adds declarations of local variables, consts and functions.


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

[Kevin Millikin (Chromium), 2009-11-05 16:25:25]

http://codereview.chromium.org/368005/diff/1/6
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/368005/diff/1/6#newcode335
Line 335: __ ldr(r0, MemOperand(sp));
__ pop(r0)?

http://codereview.chromium.org/368005/diff/1/6#newcode350
Line 350: __ Check(eq, "Not in a function context.");
The string is not informative.  Try: "Hey! You're trying to declare *your*
variable in *someone else's* context!".

Just kidding, try something short but informative.

http://codereview.chromium.org/368005/diff/1/6#newcode357
Line 357: __ ldr(r0, MemOperand(sp));
__ pop(r0) ?

http://codereview.chromium.org/368005/diff/1/3
File src/compiler.cc (right):

http://codereview.chromium.org/368005/diff/1/3#newcode654
Line 654: // All declarations are supported.
No need for the comment.  It's misleading and it's not currently true.

Right now, if there is a declaration for a fucntion that can't be compiled
lazily the call to ProcessExpression will bail out.

http://codereview.chromium.org/368005/diff/1/2
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/368005/diff/1/2#newcode306
Line 306: PropertyAttributes attr = decl->mode() == Variable::VAR ?
*Ouch*, this is almost unreadable due to the line break.  I would either put the
whole thing on the same line (break the line at the lowest precedence operator):

PropertyAttributes attr =
    (decl->mode() == Variable::VAR) ? NONE : READ_ONLY;

or else I would break before (not after) '?' and ':" and put both alternatives
on their own line:

PropertyAttributes attr = (decl->mode() == Variable::VAR)
    ? NONE
    : READ_ONLY;

http://codereview.chromium.org/368005/diff/1/2#newcode339
Line 339: __ mov(ebx, CodeGenerator::ContextOperand(
I don't really like this line break either.  Does it work to break at a comma? 
To name the operand?

http://codereview.chromium.org/368005/diff/1/2#newcode346
Line 346: ASSERT(Heap::InNewSpace(*Factory::the_hole_value()));
I guess this assert is overly defensive.  We put the hole value in code all the
time, so nothing would work if it wasn't.

http://codereview.chromium.org/368005/diff/1/2#newcode359
Line 359: __ RecordWrite(ebx, offset, eax, ecx);
We should try to make sure we have tests that hit this case.

ebx is the wrong register.  The context is in esi (and only also in abx in the
FLAG_debug_code case).

http://codereview.chromium.org/368005/diff/1/4
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/368005/diff/1/4#newcode366
Line 366: __ RecordWrite(rbx, offset, rax, rcx);
rsi, not rbx.

[William Hesse, 2009-11-05 16:34:18]

LGTM.

[fschneider, 2009-11-05 17:11:54]

http://codereview.chromium.org/368005/diff/1/6
File src/arm/fast-codegen-arm.cc (right):

http://codereview.chromium.org/368005/diff/1/6#newcode335
Line 335: __ ldr(r0, MemOperand(sp));
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> __ pop(r0)?

Done.

http://codereview.chromium.org/368005/diff/1/6#newcode350
Line 350: __ Check(eq, "Not in a function context.");
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> The string is not informative.  Try: "Hey! You're trying to declare *your*
> variable in *someone else's* context!".
> 
> Just kidding, try something short but informative.
> 
Even shorter? e.g. "gotcha!" :)

http://codereview.chromium.org/368005/diff/1/6#newcode357
Line 357: __ ldr(r0, MemOperand(sp));
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> __ pop(r0) ?

Done.

http://codereview.chromium.org/368005/diff/1/3
File src/compiler.cc (right):

http://codereview.chromium.org/368005/diff/1/3#newcode654
Line 654: // All declarations are supported.
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> No need for the comment.  It's misleading and it's not currently true.
> 
> Right now, if there is a declaration for a fucntion that can't be compiled
> lazily the call to ProcessExpression will bail out.

Done.

http://codereview.chromium.org/368005/diff/1/2
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/368005/diff/1/2#newcode306
Line 306: PropertyAttributes attr = decl->mode() == Variable::VAR ?
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> *Ouch*, this is almost unreadable due to the line break.  I would either put
the
> whole thing on the same line (break the line at the lowest precedence
operator):
> 
> PropertyAttributes attr =
>     (decl->mode() == Variable::VAR) ? NONE : READ_ONLY;
> 
> or else I would break before (not after) '?' and ':" and put both alternatives
> on their own line:
> 
> PropertyAttributes attr = (decl->mode() == Variable::VAR)
>     ? NONE
>     : READ_ONLY;

Done.

http://codereview.chromium.org/368005/diff/1/2#newcode339
Line 339: __ mov(ebx, CodeGenerator::ContextOperand(
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> I don't really like this line break either.  Does it work to break at a comma?

> To name the operand?

Done.

http://codereview.chromium.org/368005/diff/1/2#newcode346
Line 346: ASSERT(Heap::InNewSpace(*Factory::the_hole_value()));
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> I guess this assert is overly defensive.  We put the hole value in code all
the
> time, so nothing would work if it wasn't.

Done.

http://codereview.chromium.org/368005/diff/1/2#newcode359
Line 359: __ RecordWrite(ebx, offset, eax, ecx);
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> We should try to make sure we have tests that hit this case.
> 
> ebx is the wrong register.  The context is in esi (and only also in abx in the
> FLAG_debug_code case).

Done.

http://codereview.chromium.org/368005/diff/1/4
File src/x64/fast-codegen-x64.cc (right):

http://codereview.chromium.org/368005/diff/1/4#newcode366
Line 366: __ RecordWrite(rbx, offset, rax, rcx);
On 2009/11/05 16:25:25, Kevin Millikin wrote:
> rsi, not rbx.

Good catch. Done.