Thanks for the comments. I'll hold off on trying to find the right
abstractions
until I'm done deleting code.
http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc#newcode3720
src/hydrogen.cc:3720: instr->set_position(expr->position());
On 2010/12/09 12:01:48, fschneider wrote:
It seems easy to forget setting the source position. Can the
ReturnInstruction
take it as an argument? Or maybe can you easily assert inside
ReturnInstruction
that all HInstructions that have require a position have it set?
I agree. I removed one of the overloaded PushAndAdd functions because
it was easy to call the wrong one.
I don't have a good solution but I see the problem. Hopefully when more
code is deleted we'll see a simple way to do it.
http://codereview.chromium.org/5620007/diff/1/src/hydrogen.cc#newcode4303
src/hydrogen.cc:4303: if (ast_context()->IsEffect())
AddSimulate(expr->id());
On 2010/12/09 12:01:48, fschneider wrote:
Shouldn't this be
if (ast_context()->IsTest()) ...
then?
Good catch. It's the comment that's wrong.
http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.cc#newcode2063
src/hydrogen.cc:2063: HValue* true_value = invert_true()
On 2010/12/09 11:40:08, Kasper Lund wrote:
Add a helper for this pattern. GetConstantBoolean(true|false).
I think I want to get rid of all occurrences of it with my next change,
so I'll leave it.
http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h
File src/hydrogen.h (right):
http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode563
src/hydrogen.h:563: // functions for expressions.
On 2010/12/09 11:40:08, Kasper Lund wrote:
I guess you can only 'fill' a context once. Would it make sense to
verify that
this is the way it is used by having a bool is_filled_ in debug mode?
I'm not sure about that---it depends on how much state contexts hold.
For instance, we might decide that the context surrounding 'e0 ? e1 :
e2' gets filled twice.
I'll hold off on asserting anything.
http://codereview.chromium.org/5620007/diff/4001/src/hydrogen.h#newcode672
src/hydrogen.h:672: HEnvironment* environment() const { return
subgraph()->environment(); }
On 2010/12/09 11:40:08, Kasper Lund wrote:
Long term maybe this should be Environment?
Probably. Or else we should get rid of the two or three layers of
inlined helpers, track the current running environment in the graph
builder (there's no great reason for it to be in the subgraph) and write
directly:
env()->Push(...)
or whatever.
http://codereview.chromium.org/5620007/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
