I don't understand half of what is going on here. A high-level comment giving an overview would be good.
Lgtm, except for the char* stuff. http://codereview.chromium.org/12900/diff/1/14 File src/jsregexp.cc (right): http://codereview.chromium.org/12900/diff/1/14#newcode1006 Line 1006: Handle<Object> code = macro_assembler_->GetCode(*(pattern->ToCString())); ToCString? That's not usually something we'd want to use outside debug code. http://codereview.chromium.org/12900/diff/1/14#newcode1158 Line 1158: return true; Sooo many special cases, I don't understand half of what's going on here. Is there any way to divide this function up into several pieces that can be factored out into separate functions and understood separately? http://codereview.chromium.org/12900/diff/1/14#newcode1346 Line 1346: // load below. How about a high-level comment that explains what the deal is with unchecked loads? http://codereview.chromium.org/12900/diff/1/14#newcode1664 Line 1664: return kNoTextLength; This indicates that this method doesn't just return the length of text nodes, there's something more going on. How about renaming it to something more descriptive? http://codereview.chromium.org/12900/diff/1/14#newcode2948 Line 2948: EnsureAnalyzed(that->submatch_body()); Again, this special case indicates to me that a submatch should really be a separate type of node. http://codereview.chromium.org/12900/diff/1/14#newcode3277 Line 3277: target->Accept(this); ...and again. http://codereview.chromium.org/12900/diff/1/7 File src/jsregexp.h (right): http://codereview.chromium.org/12900/diff/1/7#newcode705 Line 705: static ActionNode* BeginSubmatch( This smells more like a new node than an action. http://codereview.chromium.org/12900/diff/1/7#newcode918 Line 918: class PostponedCapture :public PostponedAction { Space after : http://codereview.chromium.org/12900/diff/1/13 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/12900/diff/1/13#newcode407 Line 407: Handle<Object> RegExpMacroAssemblerIA32::GetCode(char* source) { char*? That doesn't look right. They can contain \0s you know. http://codereview.chromium.org/12900 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
