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.
On 2010/09/21 08:19:28, kmillikin wrote:
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.
They are now grouped so that the 4 methods for each context class are
always together. This means more code duplication, but hopefully more
clarity.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode280
src/arm/full-codegen-arm.cc:280: }
On 2010/09/21 08:19:28, kmillikin wrote:
// Nothing to do.
here or else consistently leave them out.
I'm going to leave them out. It's more obvious in a function than it
was in the swich statements.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode398
src/arm/full-codegen-arm.cc:398: Label* materialize_false) const {
On 2010/09/21 08:19:28, kmillikin wrote:
I think the intent is that materialize_true == true_label_ &&
materialize_false
== false_label_. Can we assert that?
Done.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode1629
src/arm/full-codegen-arm.cc:1629: context()->Plug(r0);
On 2010/09/21 08:19:28, kmillikin wrote:
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.
Done.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode2976
src/arm/full-codegen-arm.cc:2976: {
On 2010/09/21 08:19:28, kmillikin wrote:
The other times this pattern appeared you used
{ EffectContext context(this);
...
Done.
http://codereview.chromium.org/3449004/diff/10001/11001#newcode3024
src/arm/full-codegen-arm.cc:3024: ASSERT(!context()->IsEffect());
On 2010/09/21 08:19:28, kmillikin wrote:
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.
This is pretty marginal, but I'm going to leave it as it was, simply
because there isn't an Is... method for the other two states.
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
On 2010/09/21 08:19:28, kmillikin wrote:
I think this comment is confusing now. Delete it?
Done.
http://codereview.chromium.org/3449004/diff/10001/11003#newcode711
src/full-codegen.cc:711: codegen()->Visit(expr->left());
On 2010/09/21 08:19:28, kmillikin wrote:
Probably a comment here that we want the value in the accumulator for
the test
and on the stack in case we need it.
Done.
http://codereview.chromium.org/3449004/diff/10001/11003#newcode732
src/full-codegen.cc:732:
codegen()->VisitForAccumulatorValue(expr->left());
On 2010/09/21 08:19:28, kmillikin wrote:
Same comment about why we need two copies of the value.
Done.
http://codereview.chromium.org/3449004/diff/10001/11004
File src/full-codegen.h (right):
http://codereview.chromium.org/3449004/diff/10001/11004#newcode497
src/full-codegen.h:497: virtual void Plug(Slot* slot) const = 0;
On 2010/09/21 08:19:28, kmillikin wrote:
These four functions should move up to right under Plug(Register) so
they can
share the comment there.
Done.
http://codereview.chromium.org/3449004/diff/10001/11004#newcode512
src/full-codegen.h:512: // Set up branch labels for a test expression.
On 2010/09/21 08:19:28, kmillikin wrote:
Maybe the comment should say if_true, if_false, and fall_through are
output
parameters?
Done.
http://codereview.chromium.org/3449004/diff/10001/11004#newcode553
src/full-codegen.h:553: virtual void PrepareTest(Label* t,
On 2010/09/21 08:19:28, kmillikin wrote:
Might as well spell the parameters out.
Done.
http://codereview.chromium.org/3449004/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
