On 2009/10/15 14:54:27, Kevin Millikin wrote: > 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? DONE. > 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. DONE. > 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. DONE. Yes, we should confirm this. For now I just did the same thing as the normal compiler. > 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. DONE. > 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. DONE. http://codereview.chromium.org/271102 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
