LGTM - good work. Can't wait to see how it works in practice. I haven't checked all the assembler output for correctness. We need to get some tests running on this stuff.
http://codereview.chromium.org/11271/diff/201/21 File regexp2000/src/SConscript (right): http://codereview.chromium.org/11271/diff/201/21#newcode46 Line 46: 'property.cc', 'regexp-macro-assembler.cc', Please reformat. http://codereview.chromium.org/11271/diff/201/23 File regexp2000/src/assembler-ia32.h (right): http://codereview.chromium.org/11271/diff/201/23#newcode446 Line 446: void push(Label* label, RelocInfo::Mode relocation_mode = RelocInfo::NONE); This should be without a default argument to help overloading resolution. We use default arguments very sparingly (where a call is frequently used and the default is blindingly obvious to any user. http://codereview.chromium.org/11271/diff/201/27 File regexp2000/src/heap.cc (right): http://codereview.chromium.org/11271/diff/201/27#newcode1628 Line 1628: Handle<Code>* self) { As discussed offline this should be passed by value and it needs to be hooked into Factory::NewCode. http://codereview.chromium.org/11271/diff/201/27#newcode1655 Line 1655: code->CopyFrom(desc); // migrate generated code A comment here to the effect that CopyFrom may make use of the location of the self handle when relocating when moving code into the code object. http://codereview.chromium.org/11271/diff/201/28 File regexp2000/src/heap.h (right): http://codereview.chromium.org/11271/diff/201/28#newcode563 Line 563: Handle<Code>* self = NULL); We shouldn't mix handle and non-handle code. This should probably be a Code**. A comment as to it's meaning would be a good idea here. http://codereview.chromium.org/11271/diff/201/29 File regexp2000/src/jsregexp.cc (right): http://codereview.chromium.org/11271/diff/201/29#newcode2295 Line 2295: if (FLAG_re2k_native) { We should probably either ignore the flag on ARM or not have it at all. This looks like it will crash in release mode if the flag is set on ARM? http://codereview.chromium.org/11271/diff/201/29#newcode2301 Line 2301: // FIXME. Thuis should be a TODO with a name or issue number. http://codereview.chromium.org/11271/diff/201/29#newcode2302 Line 2302: // Don't compile *here*, we don't know the input type yet! This won't work! For now we just do 16 bit always for the bytecode version. http://codereview.chromium.org/11271/diff/201/30 File regexp2000/src/objects.h (right): http://codereview.chromium.org/11271/diff/201/30#newcode2120 Line 2120: // No more than eight kinds. The value currently encoded in three bits in Please add an assert somewhere. http://codereview.chromium.org/11271/diff/201/31 File regexp2000/src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/11271/diff/201/31#newcode53 Line 53: * The stack is expected to have the following structure (tentative): Be bold, not tentative! :-) http://codereview.chromium.org/11271/diff/201/31#newcode347 Line 347: NULL, Indentation! http://codereview.chromium.org/11271/diff/201/31#newcode414 Line 414: __ j(above, &no_preempt); We don't have to fix this now, but this needs moving out of line. We push backtracks a lot and this will cause too much bloat. http://codereview.chromium.org/11271/diff/201/31#newcode469 Line 469: __ jmp(&success_label_); Do we need to set eax here? http://codereview.chromium.org/11271/diff/201/35 File regexp2000/test/cctest/test-regexp.cc (right): http://codereview.chromium.org/11271/diff/201/35#newcode277 Line 277: const char* kNothingToRepeat = "Nothing to repeat"; Surely 'static const'? http://codereview.chromium.org/11271 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
