Irregexp:...

Irregexp:
* Facility for generating a node several ways.  This allows
  code to be generated for a node knowing where it is trying
  to match relative to the 'current position' and it allows
  code to be generated that knows where to backtrack to.  Both
  allow dramatic reductions in the amount of popping and pushing
  on the stack and the number of indirect jumps.
* Generate special backtracking for greedy quantifiers on
  constant-length atoms.  This allows .* to run in constant
  space relative to input string size.
* When we are checking a long sequence of characters or character
  classes in the input then we do them right to left and only the
  first (rightmost) needs to check for end-of-string.
* Record the pattern in the profile instead of just <CompiledRegExp>
* Nodes no longer contain an on_failure_ node.  This was only used
  for lookaheads and they are now handled with a choice node instead.

Committed: http://code.google.com/p/v8/source/detail?r=930

[Christian Plesner Hansen, 2008-12-03 15:10:10]

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.

[Christian Plesner Hansen, 2008-12-03 17:26:10]

On second thought, can you please not submit this until we've talked about it
tomorrow.

[Christian Plesner Hansen, 2008-12-08 08:54:18]

Does it lint?

http://codereview.chromium.org/12900/diff/203/216
File src/jsregexp.cc (right):

http://codereview.chromium.org/12900/diff/203/216#newcode1009
Line 1009: //   is requested for an identical GenerationVariant.
Most excellent!

http://codereview.chromium.org/12900/diff/203/216#newcode1205
Line 1205: int max_register,
Indentation off.

http://codereview.chromium.org/12900/diff/203/216#newcode1298
Line 1298: // Create a new trivial state and generate the node with that.
trivial -> empty?

http://codereview.chromium.org/12900/diff/203/216#newcode1443
Line 1443: Guard* guard,
Indentation.

http://codereview.chromium.org/12900/diff/203/216#newcode1599
Line 1599: ranges->at(0).from() == 0 &&
Maybe add an IsEverything method on CharacterRange to match the
CharacterRange::Everything() method that is used to construct most of these.

http://codereview.chromium.org/12900/diff/203/209
File src/jsregexp.h (right):

http://codereview.chromium.org/12900/diff/203/209#newcode914
Line 914: class GenerationVariant {
Not a fan of this name though I can't think of anything better.