In the toplevel compiler, shift the responsibility of assigning a...

In the toplevel compiler, shift the responsibility of assigning a
location to an Expression AST node from the node's parent to the node
itself.

This allows an inherited code generation context from a parent node to
be passed arbitrarily far down the tree (eg, the subexpression of a
unary not is in the same context as the unary expression itself, the
then and else subexpressions of the ternary operator are in the same
context as the whole expression, and so forth).

We do not yet take advantage of this in the backend (eg, the right
subexpression of short-circuited OR is still compiled by using the
parent's destination location, rather than the subexpression's
itself).


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

[fschneider, 2009-10-28 10:06:01]

LGTM.

Few small comments.

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

http://codereview.chromium.org/340005/diff/1/4#newcode51
Line 51: location_(Location::Nowhere()) {
I'm not sure: Should this match the default location for ast nodes(TEMP)?

http://codereview.chromium.org/340005/diff/1/4#newcode778
Line 778: Visit(expr->key());
Also set location here:

expr->set_location(location_);

http://codereview.chromium.org/340005/diff/1/4#newcode814
Line 814: }
Also set location here: expr->set_location(location_);

[William Hesse, 2009-10-28 10:28:54]

LGTM.

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

http://codereview.chromium.org/340005/diff/1/4#newcode666
Line 666: }
Control flow is tricky here.  Reversing sense of slot != NULL makes this an if,
else if that is easier to read.

[Kevin Millikin (Chromium), 2009-10-28 11:39:38]

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

http://codereview.chromium.org/340005/diff/1/4#newcode51
Line 51: location_(Location::Nowhere()) {
On 2009/10/28 10:06:01, fschneider wrote:
> I'm not sure: Should this match the default location for ast nodes(TEMP)?

Possibly.  I wanted it to be nowhere when not inside an expression (visiting a
statement or declaration).  Now that every AST node gets a location explicitly
assigned, it might make sense to introduce an INVALID or UNITIALIZED one as
expression default to catch failure to assign.

I would do that as a separate change.

http://codereview.chromium.org/340005/diff/1/4#newcode666
Line 666: }
On 2009/10/28 10:28:54, William Hesse wrote:
> Control flow is tricky here.  Reversing sense of slot != NULL makes this an
if,
> else if that is easier to read.

I see what you mean.  I've changed it.

http://codereview.chromium.org/340005/diff/1/4#newcode778
Line 778: Visit(expr->key());
On 2009/10/28 10:06:01, fschneider wrote:
> Also set location here:
> 
> expr->set_location(location_);

You're right.  Svn update can't rewrite these :)