This looks really good. A have some small comments you should address,
which
mostly apply to all three platforms.
http://codereview.chromium.org/3449004/diff/10001/11001
File src/arm/full-codegen-arm.cc (right):
http://codereview.chromium.org/3449004/diff/10001/11001#newcode257
src/arm/full-codegen-arm.cc:257: // Nothing to do.
It's not obvious why some of these are in the platform-specific and some
in the platform-independent implementation file.
My personal preference would be to have them all in the
platform-specific files. If not, then all the platform-independent
implementations should move to the platform-independent file.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode280
src/arm/full-codegen-arm.cc:280: }
// Nothing to do.
here or else consistently leave them out.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode398
src/arm/full-codegen-arm.cc:398: Label* materialize_false) const {
I think the intent is that materialize_true == true_label_ &&
materialize_false == false_label_. Can we assert that?
http://codereview.chromium.org/3449004/diff/10001/11001#newcode1629
src/arm/full-codegen-arm.cc:1629: context()->Plug(r0);
I'd probably move the two calls to context()->Plug(r0) out if the
if...else so it's obvious they're in tail position where they belong.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode2976
src/arm/full-codegen-arm.cc:2976: {
The other times this pattern appeared you used
{ EffectContext context(this);
...
http://codereview.chromium.org/3449004/diff/10001/11001#newcode3024
src/arm/full-codegen-arm.cc:3024: ASSERT(!context()->IsEffect());
I'm not sure which is more brittle to catch future changes: blacklisting
effect and test, or whitelisting the two value contexts. I'd probably
prefer the latter.
http://codereview.chromium.org/3449004/diff/10001/11003
File src/full-codegen.cc (right):
http://codereview.chromium.org/3449004/diff/10001/11003#newcode691
src/full-codegen.cc:691: // Set up the appropriate context for the left
subexpression based
I think this comment is confusing now. Delete it?
http://codereview.chromium.org/3449004/diff/10001/11003#newcode711
src/full-codegen.cc:711: codegen()->Visit(expr->left());
Probably a comment here that we want the value in the accumulator for
the test and on the stack in case we need it.
http://codereview.chromium.org/3449004/diff/10001/11003#newcode732
src/full-codegen.cc:732:
codegen()->VisitForAccumulatorValue(expr->left());
Same comment about why we need two copies of the value.
http://codereview.chromium.org/3449004/diff/10001/11004
File src/full-codegen.h (right):
http://codereview.chromium.org/3449004/diff/10001/11004#newcode492
src/full-codegen.h:492: // Emit code to convert pure control flow to a
pair of labels into the
Probably this should say "pair of unbound labels" because the
implementation may decide to bind either of them.
http://codereview.chromium.org/3449004/diff/10001/11004#newcode497
src/full-codegen.h:497: virtual void Plug(Slot* slot) const = 0;
These four functions should move up to right under Plug(Register) so
they can share the comment there.
http://codereview.chromium.org/3449004/diff/10001/11004#newcode512
src/full-codegen.h:512: // Set up branch labels for a test expression.
Maybe the comment should say if_true, if_false, and fall_through are
output parameters?
http://codereview.chromium.org/3449004/diff/10001/11004#newcode553
src/full-codegen.h:553: virtual void PrepareTest(Label* t,
Might as well spell the parameters out.
http://codereview.chromium.org/3449004/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
