Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(292)

Issue 14190: * Added min/max match length to irregexp ast (Closed)

Created:
12 years ago by Lasse Reichstein
Modified:
9 years, 6 months ago
Reviewers:
Erik Corry
CC:
v8-dev
Visibility:
Public.

Description

Each RegExtTree node can now report the min and max size of strings it can match.

Patch Set 1 #

Patch Set 2 : Now diff'ed relative to head #

Total comments: 9

Patch Set 3 : Addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+188 lines, -33 lines) Patch
M src/ast.h View 1 2 10 chunks +63 lines, -24 lines 0 comments Download
M src/ast.cc View 1 2 2 chunks +31 lines, -1 line 0 comments Download
M src/jsregexp.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/jsregexp.cc View 1 2 3 chunks +12 lines, -2 lines 0 comments Download
M src/parser.cc View 1 2 3 chunks +5 lines, -6 lines 0 comments Download
M test/cctest/test-regexp.cc View 1 2 2 chunks +76 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
Lasse Reichstein
Review please. Should we delay computation until we need it instead?
12 years ago (2008-12-17 09:48:56 UTC) #1
Erik Corry
12 years ago (2008-12-17 10:16:03 UTC) #2
LGTM

http://codereview.chromium.org/14190/diff/208/8
File src/ast.h (right):

http://codereview.chromium.org/14190/diff/208/8#newcode1222
Line 1222: virtual int min_match() = 0;
Could we call this min_chars instead?  Seems less ambiguous to me.

http://codereview.chromium.org/14190/diff/208/8#newcode1238
Line 1238: min_match_(alternatives->at(0)->min_match()),
I think these initializations should be moved into the body of the constructor,
since they will be overwritten later.

http://codereview.chromium.org/14190/diff/208/8#newcode1241
Line 1241: RegExpTree* alternative = alternatives->at(i);
We have Min and Max in utils.h for stuff like this.  Perhaps with the help of
those we can get this down to a size where it is not unreasonable to have it in
the .h file.

http://codereview.chromium.org/14190/diff/208/8#newcode1246
Line 1246: if (max_match_ != kInfinity) {
You don't need to special case infinity here.

http://codereview.chromium.org/14190/diff/208/8#newcode1299
Line 1299: 
Reordering stuff like this makes the change hard to read.

http://codereview.chromium.org/14190/diff/208/8#newcode1382
Line 1382: void AddElement(TextElement elm) {
I think TextElement should grow a Length() method.

http://codereview.chromium.org/14190/diff/208/8#newcode1392
Line 1392: UNIMPLEMENTED();
unreachable

http://codereview.chromium.org/14190/diff/208/8#newcode1409
Line 1409: min_match_(min * body->min_match()),
move into body

http://codereview.chromium.org/14190/diff/208/8#newcode1411
Line 1411: if (max > 0 && body->max_match() > kInfinity / max) {
Please add a test with a max of 0.

Powered by Google App Engine
This is Rietveld 408576698