Closed Bug 465862 Opened 14 years ago Closed 14 years ago

Native regexps: case-insensitive comparison bug

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

Details

(Keywords: verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files)

Attached patch PatchSplinter Review
Due to a 1-line typo, native matchers generated with the 'i' flag perform a bogus "to-uppercase" action on non-letters. The result is that in the following code with the -j option turned on, all 4 match operations match.

print("must match:");
print('@'.match(/@/i));
print('`'.match(/`/i));

print("must not match:");
print('@'.match(/`/i));
print('`'.match(/@/i));
Attachment #349092 - Flags: review?(gal)
Comment on attachment 349092 [details] [diff] [review]
Patch

>diff -r d5e92772e8a1 js/src/jsregexp.cpp
>+            if (L'A' <= ch && ch <= L'Z' || L'a' <= ch && ch <= L'z') {

Drive-by: Please parenthesize the && expressions against the || to avoid GCC warnings.
Flags: blocking1.9.1?
Comment on attachment 349092 [details] [diff] [review]
Patch

See Blake's drive-by nit.
Attachment #349092 - Flags: review?(gal) → review+
Pushed to TM as 21807:65019ca2ece8.
needs a test
Flags: blocking1.9.1? → blocking1.9.1+
I think a JS regression test would be appropriate. What dir would it go in? js1_1 because regexps have been around forever, or a new regexp dir, or what? I don't know the location conventions.
ecma_3/RegExp would be fine if it doesn't use any weirdness other than turning jit on/off via jit(true)/jit(false).
Attachment #349358 - Flags: review?
Attachment #349358 - Flags: review? → review?(bclary)
Comment on attachment 349358 [details] [diff] [review]
Regression test case

note the test passes but nothing appears to be traced from what I can tell. Thanks for the test. Do you want to check it in or would you like me to? I normally check into both CVS trunk and mozilla-central.
Attachment #349358 - Flags: review?(bclary) → review+
(In reply to comment #8)
> (From update of attachment 349358 [details] [diff] [review])
> note the test passes but nothing appears to be traced from what I can tell.

Correct. The regexp->native compiler shares the js option and some of the machinery with tracing but does not in fact trace. 

> Thanks for the test. Do you want to check it in or would you like me to? I
> normally check into both CVS trunk and mozilla-central.

Hmmm. I think I'll check it in to TraceMonkey for now because that's what I did with the actual fix, and let them be picked up together. But if you want to, certainly feel free to check it in to the others now.

Pushed to TM as 5f3dfe048606.
Whiteboard: fixed-tm
Whiteboard: fixed-tm → [fixed-tracemonkey]
Whiteboard: [fixed-tracemonkey] → fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
sync mc with cvs.
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-465862.js,v  <--  regress-465862.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.