On Wed, Dec 3, 2008 at 4:10 PM, <[EMAIL PROTECTED]> wrote: > I don't understand half of what is going on here. A high-level comment > giving an overview would be good.
Fixed. > > > 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. Fixed. > > > 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? Fixed. > > > 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? Fixed. > > > 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? Fixed. > > > 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. It turns out that a negative submatch success is a sort of EndNode and a positive submatch success is a sort of ActionNode. Going into the submatch it turns out that beginning a positive submatch is a sort of ActionNode and beginning a negative submatch is an ActionNode followed by a ChoiceNode. > > > http://codereview.chromium.org/12900/diff/1/14#newcode3277 > Line 3277: target->Accept(this); > ...and again. Fixed > > > 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. Fixed. > > > http://codereview.chromium.org/12900/diff/1/7#newcode918 > Line 918: class PostponedCapture :public PostponedAction { > Space after : Fixed. > > > 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. Fixed. Though the profiling API still uses char* so at the point where we log the code address there's still a char*. > > > http://codereview.chromium.org/12900 > New version uploaded. -- Erik Corry, Software Engineer Google Denmark ApS. CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
