Addressed review comments
http://codereview.chromium.org/17416/diff/28/232 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/17416/diff/28/232#newcode627 Line 627: __ mov(ecx, Operand(ebp, kStackHighEnd)); On 2009/01/09 05:44:13, iposva wrote: > How about using backtrack_stackpointer() instead of ecx here? Done. Good catch! http://codereview.chromium.org/17416/diff/28/232#newcode708 Line 708: __ bind(&stack_overflow); Yes, this is correct. It's unlikely, but perhaps possible in some pathological case, for the system stack to overflow while we are working on a regexp. We will notice this when we check the StackGuard. It's much more likely that we trigger the stack guard due to preemption or debugging. http://codereview.chromium.org/17416/diff/28/232#newcode738 Line 738: __ bind(&grow_failed); On 2009/01/09 05:44:13, iposva wrote: > You could consolidate this label with the stack_overflow label from above and > name it exit_with_exception. Done. http://codereview.chromium.org/17416/diff/28/232#newcode987 Line 987: Register RegExpMacroAssemblerIA32::backtrack_stackpointer() { I concur on the accessors, but I prefer to keep all the code-generating functions in the .cc-file, even if they are small. http://codereview.chromium.org/17416/diff/28/232#newcode1043 Line 1043: Operand(backtrack_stackpointer(), -kPointerSize)); I'm assuming the version below to be slightly more efficient (and four bytes shorter), since it doesn't need a displacement on the mov-operation. I don't actually push the backtrack-stackpointer, so this isn't (currently) necessary. I'll just make it not work for the backtrack stackpointer instead, to reduce complexity. Also, it doesn't matter whether we affect flags, so I might as well use add and sub instead of lea. http://codereview.chromium.org/17416/diff/28/237 File src/regexp-stack.h (right): http://codereview.chromium.org/17416/diff/28/237#newcode44 Line 44: // Gives the top of the memory used as stack. On 2009/01/09 05:44:13, iposva wrote: > Please use empty line between last piece of code and comment for next function > declaration to add structure for the reader's eye. Done. http://codereview.chromium.org/17416/diff/28/237#newcode46 Line 46: ExecutionAccess lock; Erik concurs. All code using these functions are guaranteed to have the global V8 lock at the time. http://codereview.chromium.org/17416 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
