regexp with ? matches incorrectly

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P3
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Robert Ginda, Assigned: rogerl (gone))

Tracking

({js1.5})

Trunk
x86
Linux
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
js> str = "foobar"
js> str.match (/(\S+)? ?(.*)/);
foobar,fooba,r
js> str = "foo bar"
js> str.match (/(\S+)? ?(.*)/);
foo bar,foo,bar

The first match should have been "foobar, foobar", the second match is correct. 
This used to work.  Maybe something missed in rogerl's 3.33 revision if
jsregexp.c?
(Reporter)

Comment 1

18 years ago
*** Bug 56249 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 2

18 years ago
*** Bug 56249 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

18 years ago
DOTSTAR was behaving as if no match was a failed match. Here's a patch:

Index: jsregexp.c
===================================================================
RCS file: /m/pub/mozilla/js/src/jsregexp.c,v
retrieving revision 3.33
diff -u -r3.33 jsregexp.c
--- jsregexp.c  2000/09/14 22:39:21     3.33
+++ jsregexp.c  2000/10/24 15:39:35
@@ -1865,7 +1865,6 @@
           for (cp2 = cp; cp2 < cpend; cp2++)
               if (*cp2 == '\n')
                   break;
-          if (cp2 == cp) return NULL;
           while (cp2 >= cp) {
               const jschar *cp3 = matchRENodes(state, ren->next, NULL, cp2);
               if (cp3 != NULL) {
(Reporter)

Comment 4

18 years ago
works here, r=rginda.
(Assignee)

Updated

18 years ago
Keywords: js1.5
(Assignee)

Comment 5

18 years ago
Created attachment 20347 [details] [diff] [review]
new mega patch fixes all RE bugs

Comment 6

18 years ago
<JS_ASSERT((ts == NULL) || (ts->linebuf.limit < ts->linebuf.base + 
JS_LINE_LIMIT));
>JS_ASSERT(!ts || (ts->linebuf.limit < ts->linebuf.base + JS_LINE_LIMIT));

maybe?
Keywords: patch, review
timeless: and don't parenthesize the < expression, this ain't Pascal.

rogerl, I'll review, someone else should too.

/be
(Assignee)

Comment 8

18 years ago
Created attachment 20376 [details] [diff] [review]
Patched personal paranthesis paranoia.
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 9

18 years ago
Added testcase to JS test suite:

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


This testcase is currently passing on SpiderMonkey, but parts of it 
are failing in Rhino. Specifically:


CASE A     pattern = /^(\S+)?( ?)(B+)$/;  //single space before second "?" 
           string = 'AAABBB';             //no spaces in given string
           actual = string.match(pattern);
           expect = Array(string, 'AAABB', cnEmptyString, 'B');


In Rhino:  string.match(pattern) === null 



CASE B     pattern = /(.+)?(!?)(!+)/;
           string = 'WOW !!! !!!';
           actual = string.match(pattern);
           expect = Array(string, 'WOW !!! !!', cnEmptyString,  '!');


In Rhino:  string.match(pattern) = ("!!!", "!!! !!!", "!",  "!!")
           This is especially strange: although matches are being produced, 
           the first term in the match array is not the given string. 



In SpiderMonkey:

All expected values are being produced. (I do not know regular expressions 
well enough to know if these are indeed what we should expect, however - )

Comment 10

18 years ago
For convenience, here is a hyperlink to the testcase (once lxr updates):

http://lxr.mozilla.org/mozilla/source/js/tests/ecma_3/RegExp/regress-57572.js
(Assignee)

Updated

18 years ago
Blocks: 66234
(Assignee)

Comment 11

18 years ago
Generated a meta bug to capture all current R.E. bugs.
Nits:

- Tabs in the patch?  See for example

+            cp += 3;
+
state->cp = cp;
+        /* Handle empty paren */
+        if (*cp == ')') {

- else after goto is a non-sequitur:

+                    goto madeit;
+                }
+                else {

- (Same area) Why not make the error case the most indented one, by testing

+                if (argc >= 2 && !JSVAL_IS_VOID(argv[1])) { /* 'flags' defined */

instead of

+                if (argc < 2 || JSVAL_IS_VOID(argv[1])) { /* 'flags' undefined */

- (Same area) Hanging indent doesn't line up with start of actual parameters:

+                    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, 
+                                                    JSMSG_NEWREGEXP_FLAGGED);

- JSMSG_IN_NOT_OBJECT wording too different from JSMSG_BAD_INSTANCEOF_RHS. 
Compare the wording of

-MSG_DEF(JSMSG_IN_NOT_OBJECT,           27, 0, JSEXN_ERR, "target of 'in'
operator must be an object")
+MSG_DEF(JSMSG_IN_NOT_OBJECT,           27, 0, JSEXN_TYPEERR, "target of 'in'
operator must be an object")

to

-MSG_DEF(JSMSG_BAD_INSTANCEOF_RHS,      31, 1, JSEXN_ERR, "invalid instanceof
operand {0}")
+MSG_DEF(JSMSG_BAD_INSTANCEOF_RHS,      31, 1, JSEXN_TYPEERR, "invalid
instanceof operand {0}")

Also, the latter provides a decompiled version of the offending right operand. 
I think the IN message and call (in jsinterp.c) should do the same.  It's good
to say *why* the operand is invalid, and it helps to mention which operand if
possible (neither the IN nor INSTANCEOF messages do that clearly; is "target"
the right operand?).  Sorry, beating a gnat with a crowbar here.

Pick these nits and sr=brendan@mozilla.org.

/be
(Assignee)

Comment 13

18 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 14

18 years ago
Verified Fixed - ran this test suite directory on Linux, WinNT, and Mac

                      js/tests/ecma_3/RegExp/

with both debug and optimized versions of the JS shell, and got 0 errors.  
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.