Reduced the code ping-pong between the full code generator and contexts a bit.

Reduced the code ping-pong between the full code generator and contexts a bit.

* Centralized AND/OR handling, keeping related code together.

* Removed HandleExpression/HandleInNonTestContext and introduced VisitInSameContext instead, making it more obvious what's actually going on.

* Consistently use a new context when visiting the left sub-expression of an AND/OR. Note that the context stacks in the full code generator and crankshaft are still a bit out of sync for the right sub-expression.

[fschneider, 2011-05-31 10:57:12]

I'm not sure about the direction of this change. 
First, I'd really would like to see a cleanup of the ForwardBailoutStack to
simplify things. Let's discuss with Kevin how to go about it.

Since we don't bailout to before branches anymore, I think we can now remove
this construct and also get rid of PrepareForBailoutBeforeSplit.

[Kevin Millikin (Chromium), 2011-05-31 11:46:27]

I have some naming suggestions.  Otherwise, LGTM.

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

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode704
src/full-codegen.cc:704: Comment cmnt(masm_, "[ BinaryOperation");
By moving this comment into VisitComma, VisitLogical..., VisitArithmetic..., you
can emit a more descriptive comment.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode707
src/full-codegen.cc:707: case Token::OR: return VisitAndOr(expr, false);
Let's not pass the flag here, since we can just use the expression:

case Token::OR:
case Token::AND:
  return VisitLogicalOperation(expr);
...

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode745
src/full-codegen.cc:745: int rightId = expr->RightId();
right_id

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode748
src/full-codegen.cc:748: if (context()->IsTest()) {
I did like having this code as virtual functions on the context classes.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode826
src/full-codegen.cc:826: AstVisitor::Visit(expr);
This can just be Visit(expr), can't it?

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h
File src/full-codegen.h (right):

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h#newcode331
src/full-codegen.h:331: VisitInSameContext(expr);
"SameContext" is not quite right because it raises the question "same as what?".
 Can we try something like VisitInCurrentContext?

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h#newcode547
src/full-codegen.h:547: void VisitAndOr(BinaryOperation* expr, bool
is_logical_and);
VisitLogicalOperation?   We should get Logical in there (also used in the spec)
to distinguish from the bitwise operations.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h#newcode548
src/full-codegen.h:548: void VisitCommon(BinaryOperation* expr);
Common is nondescriptive.  How about ArithmeticOperation?

[Sven Panne, 2011-05-31 14:30:33]

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

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode704
src/full-codegen.cc:704: Comment cmnt(masm_, "[ BinaryOperation");
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> By moving this comment into VisitComma, VisitLogical..., VisitArithmetic...,
you
> can emit a more descriptive comment.

Done.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode707
src/full-codegen.cc:707: case Token::OR: return VisitAndOr(expr, false);
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> Let's not pass the flag here, since we can just use the expression:
> 
> case Token::OR:
> case Token::AND:
>   return VisitLogicalOperation(expr);
> ...

Done, same for HGraphBuilder.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode745
src/full-codegen.cc:745: int rightId = expr->RightId();
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> right_id

Done.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode748
src/full-codegen.cc:748: if (context()->IsTest()) {
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> I did like having this code as virtual functions on the context classes.

In principle I agree that the explicit IfFooContext() calls are not nice, but
moving the if/then/else branches to the contexts would result in a heavy feature
envy regarding the full code generator. Removing this feature envy was basically
the main motivation for this change. Furthermore, similar ifs are elsewhere, so
this is more consistent.

In a nutshell: We should improve how we handle the triple dispatch, but not in
this change.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.cc#newcode826
src/full-codegen.cc:826: AstVisitor::Visit(expr);
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> This can just be Visit(expr), can't it?

Done.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h
File src/full-codegen.h (right):

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h#newcode331
src/full-codegen.h:331: VisitInSameContext(expr);
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> "SameContext" is not quite right because it raises the question "same as
what?".
>  Can we try something like VisitInCurrentContext?

Done.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h#newcode547
src/full-codegen.h:547: void VisitAndOr(BinaryOperation* expr, bool
is_logical_and);
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> VisitLogicalOperation?   We should get Logical in there (also used in the
spec)
> to distinguish from the bitwise operations.

OK, I'm changing HGraphBuilder's corresponding method in the same way.

http://codereview.chromium.org/6976028/diff/1007/src/full-codegen.h#newcode548
src/full-codegen.h:548: void VisitCommon(BinaryOperation* expr);
On 2011/05/31 11:46:28, Kevin Millikin wrote:
> Common is nondescriptive.  How about ArithmeticOperation?

OK, I'll sync HGraphBuilder, too.