Bug 39289 - Wrong result in case of non-iterative matching of subpatterns in non-greedy cases in YARR Interpreter
Summary: Wrong result in case of non-iterative matching of subpatterns in non-greedy c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 06:41 PDT by Peter Varga
Modified: 2010-05-19 08:21 PDT (History)
6 users (show)

See Also:


Attachments
proposed patch (1.57 KB, patch)
2010-05-18 06:44 PDT, Peter Varga
darin: review-
darin: commit-queue-
Details | Formatted Diff | Diff
proposed patch v2 (5.98 KB, patch)
2010-05-19 07:32 PDT, Peter Varga
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Varga 2010-05-18 06:41:22 PDT
The start of the non-iterative matching of subpatterns isn't stored 
in case of non-greedy matching.

Eg.:
  print("ab".match(/(a)??b/));
 
Results:
  YARR Interpreter: ab,
  Expected: ab,a
Comment 1 Peter Varga 2010-05-18 06:44:59 PDT
Created attachment 56366 [details]
proposed patch
Comment 2 Darin Adler 2010-05-18 09:21:11 PDT
Comment on attachment 56366 [details]
proposed patch

Looks like a good code change. But needs a test case to demonstrate at least one regular expression that malfunctions without this and works with it. We don't take patches without test cases unless there's a reason a test case can't be created.
Comment 3 Darin Adler 2010-05-18 09:23:17 PDT
I see the test case here in the bug report. Now it needs to be added to existing tests in LayoutTests. I suggest putting it into one of the existing tests in the fast/js directory that tests other regular expression cases. And add other similar test cases that are not affected.
Comment 4 Peter Varga 2010-05-19 07:32:58 PDT
Created attachment 56489 [details]
proposed patch v2

I fixed the previous change and added a layout test. My previous change caused problems in some test cases and these cases have been added to the layout test.
Comment 5 Darin Adler 2010-05-19 07:40:28 PDT
Comment on attachment 56489 [details]
proposed patch v2

> -                    output[(subpatternId << 1) + 1] = input.getPos() + term.inputPosition;
> +                    output[(subpatternId << 1)] = input.getPos() + term.inputPosition;

I suggest omitting the parentheses here. output[subpatternId << 1] would be easier to read.

> +        The backtrackParentesesOnceEnd function should store the start position

Typo here in the name of the function.
Comment 6 Peter Varga 2010-05-19 08:01:51 PDT
Comment on attachment 56489 [details]
proposed patch v2

cq+ removed to fix the mentioned issues.
Comment 7 Andras Becsi 2010-05-19 08:21:24 PDT
(In reply to comment #6)
> (From update of attachment 56489 [details])
> cq+ removed to fix the mentioned issues.
Sending        JavaScriptCore/ChangeLog

Sending        JavaScriptCore/yarr/RegexInterpreter.cpp
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/js/regexp-non-greedy-parentheses-expected.txt
Adding         LayoutTests/fast/js/regexp-non-greedy-parentheses.html
Adding         LayoutTests/fast/js/script-tests/regexp-non-greedy-parentheses.js
Transmitting file data ......
Committed revision 59766.

Patch landed. Closing bug.