LGTM
http://codereview.chromium.org/165443/diff/42/1004 File src/ia32/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/165443/diff/42/1004#newcode1165 Line 1165: Address new_stack_top = RegExpStack::EnsureCapacity(size * 2); Shouldn't new_stack_top be new_stack_base? http://codereview.chromium.org/165443/diff/42/1006 File src/jsregexp.cc (right): http://codereview.chromium.org/165443/diff/42/1006#newcode471 Line 471: } while (res == RegExpMacroAssemblerX64::RETRY); I think you should move these enums up a level so they are the same for 32 and 64 bit. Then you could get rid of this nasty big ifdef. There's a lot of duplicate code in the two branches. http://codereview.chromium.org/165443/diff/42/1006#newcode488 Line 488: #endif // V8_TARGET_ARCH_IA32 Wrong comment. http://codereview.chromium.org/165443/diff/42/1006#newcode4505 Line 4505: RegExpMacroAssemblerX64::Mode mode; Again, lets try to keep the amount of copied code to a minimum. http://codereview.chromium.org/165443/diff/42/1013 File src/regexp-stack.h (right): http://codereview.chromium.org/165443/diff/42/1013#newcode77 Line 77: static const uint32_t kMemoryTop = 0xffffffff; Does this work on 64 bit? http://codereview.chromium.org/165443/diff/42/1016 File src/x64/regexp-macro-assembler-x64.cc (right): http://codereview.chromium.org/165443/diff/42/1016#newcode47 Line 47: * Please notice that this is the byte offset, not the character notice -> note http://codereview.chromium.org/165443/diff/42/1016#newcode64 Line 64: * Each call to a C++ method should retain these registers. Meaning of the words 'retain' and 'these' is unclear. http://codereview.chromium.org/165443/diff/42/1016#newcode121 Line 121: // Unuse labels in case we throw away the assembler without calling GetCode. I wonder whether we ever do this any more. Looks like a leftover from when we still had the pcre-based code to fall back on. http://codereview.chromium.org/165443/diff/42/1016#newcode579 Line 579: // No custom implementation (yet): w, W, s(UC16), S(UC16). We should fix this. http://codereview.chromium.org/165443/diff/42/1016#newcode605 Line 605: // MSVC passes arguments in rcx, rdx, r8, r9, with backing stack slots. This is presumable a property of the ABI and not the compiler. http://codereview.chromium.org/165443/diff/42/1016#newcode794 Line 794: #ifndef WIN64 I wonder if we can reduce the number of ifdefs with some strategic predefined macros. http://codereview.chromium.org/165443/diff/42/1016#newcode935 Line 935: __ movq(register_location(register_index), Immediate(to)); Seems like movl is enough here. Or is the smallest possible version automatically emitted? http://codereview.chromium.org/165443/diff/42/1016#newcode1308 Line 1308: // Notice: This updates flags, unlike normal Push. Replace notice with note throughout. http://codereview.chromium.org/165443/diff/42/1016#newcode1324 Line 1324: // The position succeeds a relative label offset from position. Somehow the grammar in this comment is strange. http://codereview.chromium.org/165443/diff/42/1017 File src/x64/regexp-macro-assembler-x64.h (right): http://codereview.chromium.org/165443/diff/42/1017#newcode130 Line 130: // Offsets from ebp of function parameters and stored registers. rbp? http://codereview.chromium.org/165443/diff/42/1017#newcode136 Line 136: #ifdef WIN64 I'd rather have two ifdefs than this huge one. A comment as to why there is a difference between WIN64 and non-WIN64 would be nice too. http://codereview.chromium.org/165443 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
