LGTM, with some style issues.
http://codereview.chromium.org/271102/diff/1/10 File src/arm/codegen-arm.h (right): http://codereview.chromium.org/271102/diff/1/10#newcode158 Line 158: Too much whitespace? http://codereview.chromium.org/271102/diff/1/10#newcode170 Line 170: static void RecordPositions(MacroAssembler* masm, int pos); There's a big comment in codegen.h listing all the functions that are required to be in the platform-specific CodeGenerator classes (because they're needed by clients). You should probably add this function. http://codereview.chromium.org/271102/diff/1/9 File src/arm/fast-codegen-arm.cc (right): http://codereview.chromium.org/271102/diff/1/9#newcode54 Line 54: // ARM does NOT call CodeForFunctionPosition Period at the end of the comment. We should talk to Søren when he gets back about what ARM *should* do. http://codereview.chromium.org/271102/diff/1/2 File src/fast-codegen.h (right): http://codereview.chromium.org/271102/diff/1/2#newcode52 Line 52: // These member function use CodeGenerator::recordPositions No need for this comment about the implementation. It will just have to be kept up to date whenever we change the implementation. http://codereview.chromium.org/271102/diff/1/2#newcode54 Line 54: void CodeForFunctionPosition(FunctionLiteral* fun); Since we're building a new code generator, it's probably a good time to give these better names. They don't emit code, so I think "SetFunctionPosition" is better. http://codereview.chromium.org/271102 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
