Regexp: Remove nodes from the regexp that cannot match because

Regexp: Remove nodes from the regexp that cannot match because
they contain non-ASCII characters and the input string is ASCII.
Remove unused Clone() method.
Committed: https://code.google.com/p/v8/source/detail?r=11445

[ulan, 2012-04-25 12:55:28]

LGTM if the comments are addressed.

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.cc
File src/jsregexp.cc (right):

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.cc#newcod...
src/jsregexp.cc:2722: for (int i = 0; i < choice_count; i++) {
Instead of duplicating the same loop twice, can we do in-place filtering here?

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.cc#newcod...
src/jsregexp.cc:2781: if (node == NULL) {
Did you mean replacement == NULL?

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h
File src/jsregexp.h (right):

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h#newcode582
src/jsregexp.h:582: // never match.  This method returns returns a node that can
be substituted
double "returns"

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h#newcode586
src/jsregexp.h:586: RegExpNode* Uninitialized() {
If it doesn't make big difference in speed/space then I would
prefer a separate flag that indicates whether the replacement_ was initialized
or not?

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h#newcode970
src/jsregexp.h:970: GuardedAlternative(RegExpNode* node, ZoneList<Guard*>*
guards)
This seems to be unused.

https://chromiumcodereview.appspot.com/10174017/diff/1/test/mjsunit/regexp-ca...
File test/mjsunit/regexp-capture-3.js (right):

https://chromiumcodereview.appspot.com/10174017/diff/1/test/mjsunit/regexp-ca...
test/mjsunit/regexp-capture-3.js:162: "This is an ASCII string that could take
forever".match(re);
Could you please add more tests?
At least one for each of the node types that have FilterAscii.

[Erik Corry, 2012-04-26 08:23:11]

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.cc
File src/jsregexp.cc (right):

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.cc#newcod...
src/jsregexp.cc:2722: for (int i = 0; i < choice_count; i++) {
On 2012/04/25 12:55:28, ulan wrote:
> Instead of duplicating the same loop twice, can we do in-place filtering here?


I can't see a way to do in-place filtering, since the zonelist does not have a
delete operation.  I'd rather not build a new list unless we have to since it
will cause extra allocation, and I think the typical case will be that we don't
need a new list.  I have simplified the loops instead.

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.cc#newcod...
src/jsregexp.cc:2781: if (node == NULL) {
On 2012/04/25 12:55:28, ulan wrote:
> Did you mean replacement == NULL?

Oops, yes.

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h
File src/jsregexp.h (right):

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h#newcode582
src/jsregexp.h:582: // never match.  This method returns returns a node that can
be substituted
On 2012/04/25 12:55:28, ulan wrote:
> double "returns"

done done

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h#newcode586
src/jsregexp.h:586: RegExpNode* Uninitialized() {
On 2012/04/25 12:55:28, ulan wrote:
> If it doesn't make big difference in speed/space then I would
> prefer a separate flag that indicates whether the replacement_ was initialized
> or not?

Done.

https://chromiumcodereview.appspot.com/10174017/diff/1/src/jsregexp.h#newcode970
src/jsregexp.h:970: GuardedAlternative(RegExpNode* node, ZoneList<Guard*>*
guards)
On 2012/04/25 12:55:28, ulan wrote:
> This seems to be unused.

Done.

[ulan, 2012-04-26 09:00:37]

LGTM