Refix issue 1472. The previous fix worked for the example in the bug

Refix issue 1472.  The previous fix worked for the example in the bug
report, but was not general enough to catch all cases.  This is a new
approach.  Includes regression test!
Committed: http://code.google.com/p/v8/source/detail?r=8318

[Lasse Reichstein, 2011-06-16 12:12:47]

LGTM

http://codereview.chromium.org/7193007/diff/1/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7193007/diff/1/src/jsregexp.cc#newcode3733
src/jsregexp.cc:3733: class RegExpExpansionLimiter {
Some documentation would be nice.
What problem is this class supposed to solve.

http://codereview.chromium.org/7193007/diff/1/src/jsregexp.cc#newcode3742
src/jsregexp.cc:3742: if (new_factor < saved_regexp_expansion_factor_) {
Dangerous assumption. Multiplication, if it's supposed to be that, can wrap more
than once.
Test instead:
  if (kMaxInt / factor < saved_regexp_expansion_factor_) {
    // overflow ...

http://codereview.chromium.org/7193007/diff/1/src/jsregexp.cc#newcode3808
src/jsregexp.cc:3808: Isolate::Current(), min + ((max - min) ? 1 : 0));
Do we really not have a faster way to get the isolate?

Is it important to store the expansion factor between regexp compilations (in
the isolate), or could it be stored on, e.g., the RegExpCompiler class, making
this independent of isolates? It seems it should be one value per RegExp
compiled.

[Erik Corry, 2011-06-16 15:27:28]

New version uploaded

[Lasse Reichstein, 2011-06-17 07:18:49]

LGTM

http://codereview.chromium.org/7193007/diff/3002/src/ast.h
File src/ast.h (right):

http://codereview.chromium.org/7193007/diff/3002/src/ast.h#newcode213
src/ast.h:213: Expression() : id_(GetNextId()), test_id_(GetNextId()) {}
What's the test_id for?

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3748
src/jsregexp.cc:3748: ok_to_expand_(true) {
How about initializing this as:
  ok_to_expand_(saved_expansion_factor_ <= kMaxExpansionFactor)
and not doing anything later if it's already false.

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3750
src/jsregexp.cc:3750: if (factor > kMaxExpansionFactor) {
Add comment saying that this prevents overflow in later computation, because
kMaxExpansionFactor^2 < kMaxInt.

Well, actually it doesn't if you have a very deep nesting of {6} quantifiers,
because you don't cap the stored expansion_factor. Initializing ok_to_expand as
above, and bailing out if it's already false will prevent this.

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3757
src/jsregexp.cc:3757: }
Remove the if and just always assing:
 ok_to_expand = (new_factor <= kMaxExpansionFactor);

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3818
src/jsregexp.cc:3818: compiler, min + ((max - min) ? 1 : 0));
I just noticed the using of (max - min) as a boolean. Please add a  " != 0".

[Erik Corry, 2011-06-17 07:31:13]

http://codereview.chromium.org/7193007/diff/3002/src/ast.h
File src/ast.h (right):

http://codereview.chromium.org/7193007/diff/3002/src/ast.h#newcode213
src/ast.h:213: Expression() : id_(GetNextId()), test_id_(GetNextId()) {}
On 2011/06/17 07:18:49, Lasse Reichstein wrote:
> What's the test_id for?

Don't know.

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3748
src/jsregexp.cc:3748: ok_to_expand_(true) {
On 2011/06/17 07:18:49, Lasse Reichstein wrote:
> How about initializing this as:
>   ok_to_expand_(saved_expansion_factor_ <= kMaxExpansionFactor)
> and not doing anything later if it's already false.

Done.

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3750
src/jsregexp.cc:3750: if (factor > kMaxExpansionFactor) {
On 2011/06/17 07:18:49, Lasse Reichstein wrote:
> Add comment saying that this prevents overflow in later computation, because
> kMaxExpansionFactor^2 < kMaxInt.
> 
> Well, actually it doesn't if you have a very deep nesting of {6} quantifiers,
> because you don't cap the stored expansion_factor. Initializing ok_to_expand
as
> above, and bailing out if it's already false will prevent this.

Done.

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3757
src/jsregexp.cc:3757: }
On 2011/06/17 07:18:49, Lasse Reichstein wrote:
> Remove the if and just always assing:
>  ok_to_expand = (new_factor <= kMaxExpansionFactor);

Done.

http://codereview.chromium.org/7193007/diff/3002/src/jsregexp.cc#newcode3818
src/jsregexp.cc:3818: compiler, min + ((max - min) ? 1 : 0));
On 2011/06/17 07:18:49, Lasse Reichstein wrote:
> I just noticed the using of (max - min) as a boolean. Please add a  " != 0".

max - min replaced with max != min