67773 - Regexp problem, When having (foo)+, moz ignores '+'

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[mar_garina]

(I use mozilla build 20010202 , linux)

Description of the problem:
When regexp has, for example \w+ , it means that the string should contain an
alphanumeric character at least once, but also MORE THAN ONE (that's the +'s
job). If I want a more complicated string, such as alphanumeric char and later a
'-' to show up more than once, it should be inside brackets, and then a '+'-
(\w+\-)+ .
This works great on netscape 4.x, but on mozilla it ignores the +, so 'bla-' is
accepted, but 'bla-bla-' isn't.

How it should be:
'bla-bla-' should be also accepted.

How to reproduce:
http://www.deot.net/jsbug2.html ,
I put there the exact problem.

Comment 1

[rogerl (gone)]

Nuts, another one for the regression suite. Issue here is that the $ forces the 
sub-expression to fail to run to completion, even though it has a succesful 
match half-way through. This is just the opposite case of the last '?' bugs 
where I needed the sub-expression to back-track from a failed completion. Patch 
coming up.

Comment 2

[rogerl (gone)]

Attachment: Attempt end-run and then partial sub-tree match.

Comment 3

[Phil Schwartau]

Note: the regexp '?'-character issue Roger mentioned was bug 57572 -

Comment 4

[Phil Schwartau]

Testcase added to JS test suite -

              js/tests/ecma_3/RegExp/regress-67773.js

Comment 5

[Phil Schwartau]

cc'ing potential reviewers. It would be nice to get this into JS1.5 RC3 -

Comment 6

[rogerl (gone)]

Attachment: Better, plus some clean-up.

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 69989 has been marked as a duplicate of this bug. ***

Comment 8

[Phil Schwartau]

cc'ing Stephen and Boris, contributors to bug 69989 -

Comment 9

[Brendan Eich [:brendan]]

Looks good, I'm inclined to rs= (rubberstamp= :-).  One question:

@@ -1397,15 +1396,15 @@
         }
         if ((grState->maxKid == 0) || (++grState->kidCount < grState->maxKid)) {
             match = greedyRecurse(grState, kidMatch, cp);
-            if (match) return match;
+            if (match)
+                return match;
             --grState->kidCount;
         }

Do you really want to --grState->kidCount if grState->maxKid was 0 at the if
condition?  In that case, the ++grState->kidCount never happened.  Also, what
happens if grState->kidCount >= grState->maxKid?  It'll equal once, then keep
going, so long as maxKid is non-zero and this block is reached.

/be

Comment 10

[rogerl (gone)]

Thank you; yes, that's not right. The most recent change can bring us right back 
here without resetting kidcount. I'll add a new patch.
{in fact in trying to find a test case, i found that the counted quantifier 
wasn't setting the paren value right so the new patch fixes that too)

Comment 11

[rogerl (gone)]

Attachment: Reset kidCount for second pass + fix paren set for {}.

Comment 12

[Brendan Eich [:brendan]]

sr=[e-mail].  Who should r=?  Maybe rginda or shaver?

/be

Comment 13

[Robert Ginda]

r=rginda

Comment 14

[rogerl (gone)]

Fix checked in.

Comment 15

[Phil Schwartau]

Verified Fixed on WinNT, Linux, and Mac. The testcase

           js/tests/ecma_3/RegExp/regress-67773.js

passes in the standalone JS shell on all three platforms - 

Comment 16

[Stephen Ostermiller]

My testcase from bug 69989 still does not work.  (Build ID: 2001030813 Linux)
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=26027
That bug was marked a duplicate of this bug, but the fix for this bug did not
fix that one.  I'm reopening bug 69989.

Comment 17

[Phil Schwartau]

(Note: we have found the testcase from bug 69989 does work.
 So both that bug and this one remain Verified Fixed)