Addressed comments, but I have also managed to break some tests in the process.
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', Done. Wouldn't it be easier to maintain if the file names were a vertical list instead? 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); Indeed this actually hid an error where push(0) was taken to mean this push. Done. 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) { On 2008/11/21 13:03:04, Erik Corry wrote: > As discussed offline this should be passed by value and it needs to be hooked > into Factory::NewCode. Done. http://codereview.chromium.org/11271/diff/201/27#newcode1655 Line 1655: code->CopyFrom(desc); // migrate generated code On 2008/11/21 13:03:04, Erik Corry wrote: > 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. Done. 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); On 2008/11/21 13:03:04, Erik Corry wrote: > 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. Done. 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) { On 2008/11/21 13:03:04, Erik Corry wrote: > 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? Done. http://codereview.chromium.org/11271/diff/201/29#newcode2301 Line 2301: // FIXME. On 2008/11/21 13:03:04, Erik Corry wrote: > Thuis should be a TODO with a name or issue number. Done. 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! The ia32 code will also assume 16-bit strings then, for now. 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 On 2008/11/21 13:03:04, Erik Corry wrote: > Please add an assert somewhere. Done. 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): Done! http://codereview.chromium.org/11271/diff/201/31#newcode347 Line 347: NULL, On 2008/11/21 13:03:04, Erik Corry wrote: > Indentation! > Done. http://codereview.chromium.org/11271/diff/201/31#newcode469 Line 469: __ jmp(&success_label_); No. The code at success_lable_ sets eax and runs into the exit code. Fail sets eax and jumps to the exit code. 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"; On 2008/11/21 13:03:04, Erik Corry wrote: > Surely 'static const'? Why? None of the other char*'s are static. Wouldn't it be a vaste of static memory? http://codereview.chromium.org/11271 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
