LGTM
http://codereview.chromium.org/17416/diff/245/250 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/17416/diff/245/250#newcode244 Line 244: #ifdef DEBUG This will not do. We can't generate different code in DEBUG mode than in release mode. That leads to undebuggable code generation bugs. http://codereview.chromium.org/17416/diff/245/250#newcode694 Line 694: // If returned value is non-zero, the stack guard reports the actual Surely this can't happen any more? http://codereview.chromium.org/17416/diff/245/251 File src/regexp-macro-assembler-ia32.h (right): http://codereview.chromium.org/17416/diff/245/251#newcode142 Line 142: // Adds code that loads the character at the given offset from the That loads n characters... Actually the other comments don't have the 'Add code that...' prefix. Should probably be made consistent. http://codereview.chromium.org/17416/diff/245/251#newcode151 Line 151: void CheckStackLimit(); At some point we may want to combine CheckStackLimit and CheckPreemption for efficiency. http://codereview.chromium.org/17416/diff/245/251#newcode226 Line 226: // Which mode to generate code for (ASCII or UTF16). UC16, not UTF16 http://codereview.chromium.org/17416/diff/245/254 File src/regexp-stack.cc (right): http://codereview.chromium.org/17416/diff/245/254#newcode40 Line 40: return to + size; Whoever introduced this way of archiving thread local data was an idiot :-) http://codereview.chromium.org/17416/diff/245/254#newcode63 Line 63: size = RoundUpToPowerOf2(size); I see no need to round up. On the contrary if we ever move to a growth factor that is not 2 then it could cause confusion, esp one that is less than 2. http://codereview.chromium.org/17416/diff/245/255 File src/regexp-stack.h (right): http://codereview.chromium.org/17416/diff/245/255#newcode90 Line 90: limit_(reinterpret_cast<Address>(kMemoryTop)) {} You wouldn't have to cast this if it had the right type to start with. Of course, then you'd have to cast it in the initializer, but that seems marginally nicer to me. http://codereview.chromium.org/17416/diff/245/257 File test/cctest/test-regexp.cc (right): http://codereview.chromium.org/17416/diff/245/257#newcode680 Line 680: reinterpret_cast<void*>(const_cast<T**>(input))), A reinterpret_cast in a reinterpret_cast? http://codereview.chromium.org/17416 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
