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#newcode2722 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#newcode2781 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-capture-3.js File test/mjsunit/regexp-capture-3.js (right): https://chromiumcodereview.appspot.com/10174017/diff/1/test/mjsunit/regexp-capture-3.js#newcode162 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. https://chromiumcodereview.appspot.com/10174017/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
