A few "extract method" refactorings, trying to get individual method definitions

A few "extract method" refactorings, trying to get individual method definitions
onto a sinlge 30" screen. A lot of times, the AST visitor stops a bit too early,
so we have to do the rest of the dispatch by hand. This is caused by the fact
that the kind of the AST nodes are a bit too coarse for some traversals (e.g. a
single node type for all binary ops), perhaps one could try to refine this a
little bit more.
Committed: http://code.google.com/p/v8/source/detail?r=7839

[fschneider, 2011-05-09 15:05:35]

LGTM.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4572
src/hydrogen.cc:4572: case Token::DELETE: return
VisitUnaryOperationDelete(expr);
Indent all case-clauses by 2.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4889
src/hydrogen.cc:4889: case Token::ADD:
Indent all case-clauses by 2.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4913
src/hydrogen.cc:4913: return 0;
I'd return NULL instead of 0.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4937
src/hydrogen.cc:4937: case Token::COMMA: return VisitBinaryOperationComma(expr);
Indentation.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h#newcode863
src/hydrogen.h:863: HInstruction* BuildBinaryOperation(Token::Value op, HValue*
left,
At the declaration we usually break the line for each parameter if it does not
fit on one line.

[Kevin Millikin (Chromium), 2011-05-09 15:14:05]

I'm not sure I like this change.

These functions only work when called in 'tail position' wrt visiting some
expression.

Since that's hard to enforce, and definitely not explicit in the signature, I'm
usually agressive about inlining such helpers when they have a single call site.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h#newcode738
src/hydrogen.h:738: void VisitUnaryOperationDelete(UnaryOperation* expr);
I'm not a fan of such verbose names.  I think UnaryOperation could be left
implicit: VisitDelete, VisitVoid, VisitTypeof, ..., VisitComma,
VisitLogicalOperation, VisitArithmeticOperation, ....

[Sven Panne, 2011-05-10 07:48:29]

On 2011/05/09 15:14:05, Kevin Millikin wrote:
> [...] These functions only work when called in 'tail position' wrt visiting
some expression.

I'm not sure if I completely understand this. Perhaps we can clarify this
offline/tomorrow.

> Since that's hard to enforce, and definitely not explicit in the signature,
I'm usually agressive about inlining such helpers when they have a single call
site.

Hmmm, having a single call site is usually a weak reason to inline something.
Every method should have a single responsibility and a single reason to change,
so I extracted those methods (and should fit on a 30" monitor ;-). If there is
some invariant not explicit in their signatures, we should change this, but as
already mentioned above, I fail to see what it is...

[Sven Panne, 2011-05-10 07:48:47]

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4572
src/hydrogen.cc:4572: case Token::DELETE: return
VisitUnaryOperationDelete(expr);
On 2011/05/09 15:05:35, fschneider wrote:
> Indent all case-clauses by 2.

Ooops, google.el was accidentally commented out in my ~/.emacs... Fixed.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4889
src/hydrogen.cc:4889: case Token::ADD:
On 2011/05/09 15:05:35, fschneider wrote:
> Indent all case-clauses by 2.

Done.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4913
src/hydrogen.cc:4913: return 0;
On 2011/05/09 15:05:35, fschneider wrote:
> I'd return NULL instead of 0.

I think that each project has its own ideas about what's better here, in other
projects I've been forced to do it the other way round... :-) Fixed, conforming
to the Google C++ style guide.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.cc#newcode4937
src/hydrogen.cc:4937: case Token::COMMA: return VisitBinaryOperationComma(expr);
On 2011/05/09 15:05:35, fschneider wrote:
> Indentation.

Done.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h#newcode738
src/hydrogen.h:738: void VisitUnaryOperationDelete(UnaryOperation* expr);
On 2011/05/09 15:14:05, Kevin Millikin wrote:
> I'm not a fan of such verbose names.  I think UnaryOperation could be left
> implicit: VisitDelete, VisitVoid, VisitTypeof, ..., VisitComma,
> VisitLogicalOperation, VisitArithmeticOperation, ....

Done.

http://codereview.chromium.org/6963008/diff/1/src/hydrogen.h#newcode863
src/hydrogen.h:863: HInstruction* BuildBinaryOperation(Token::Value op, HValue*
left,
On 2011/05/09 15:05:35, fschneider wrote:
> At the declaration we usually break the line for each parameter if it does not
> fit on one line.

Done.

[Sven Panne, 2011-05-10 08:33:18]

Feedback integrated, take another look, please...

[fschneider, 2011-05-10 13:34:20]

http://codereview.chromium.org/6963008/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/6963008/diff/7001/src/hydrogen.cc#newcode4864
src/hydrogen.cc:4864: HInstruction* instr = BuildBinaryOperation(expr->op(),
left, right, info);
I agree with Kevin to a degree.

Having two function BuildBinaryOperation is a little confusing. Also the
overhead of doing 3 calls for this common case seems too much.
(VisitBinaryOperation->VisitCommon->BuildBinaryOperation->BuildBinaryOperation)
We should not slow down the common case.

Helper for logical && and || seems fine. Helper for COMMA is only one line, so
not really needed.