http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (right):
http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc#newcode3744 src/ia32/full-codegen-ia32.cc:3744: // TODO(svenpanne): Allowing format strings in Comment would be nice here... OK, I'll have a look, especially if the change is really worth it, but this would be in another change, anyway. http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc#newcode3752 src/ia32/full-codegen-ia32.cc:3752: VisitForAccumulatorValue(expr->expression()); I'll investigate this later, and I wanted to do this in a separate change (if any), anyway. Is the AST really kept around for an extended period of time? If not, pulling the position into Expression (or even higher in the class hierarchy) seems to be the the right thing. Even if the position is not used for some kind of nodes, I like simple rules like "every expression has a position" instead of having crankshaft silently generate obscure code. :-) http://codereview.chromium.org/6879081/diff/10001/src/ia32/full-codegen-ia32.cc#newcode3753 src/ia32/full-codegen-ia32.cc:3753: __ CallStub(&stub); On 2011/04/27 11:34:36, fschneider wrote:
[...] It's probably good to add inlined smi code for SUB and BIT_NOT
as a separate change. OK, I'll keep this in mind... http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32.cc File src/ia32/full-codegen-ia32.cc (left): http://codereview.chromium.org/6879081/diff/16001/src/ia32/full-codegen-ia32.cc#oldcode3733 src/ia32/full-codegen-ia32.cc:3733: GenericUnaryOpStub stub(Token::SUB, overwrite, NO_UNARY_FLAGS); On 2011/04/27 11:34:36, fschneider wrote:
Please remove the code for the GenericUnaryOpStub (in code-stubs.h,
etc.), since it is not used anymore after your change. I already have exactly this on my TODO list, plus a few other minor cleanups. I want to do each cleanup in a small, separate change to avoid monster commits like this one. http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/6879081/diff/16001/src/x64/code-stubs-x64.cc#newcode455 src/x64/code-stubs-x64.cc:455: void TypeRecordingUnaryOpStub::GenerateSmiStubSub(MacroAssembler* masm) { On 2011/04/27 11:34:36, fschneider wrote:
I'm not convinced that all the short helpers like this one, that are
used exactly in one place, buy us much. Perhaps they are a little bit small, but they avoid copy-n-paste of machine code generation, and you can easily see the structure of the stubs.
If you plan to refactor these helpers anyway, it's ok to do this as a
separate change later. I have some ideas here, let's see if there's time... http://codereview.chromium.org/6879081/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
