Closed
Bug 593663
Opened 14 years ago
Closed 14 years ago
"1+2".replace("1+2", "$&+3","g"); returns "1+2", expected "1+2+3"
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: alice0775, Assigned: cdleary)
References
Details
(Keywords: regression, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Related:
Bug 592143 - TM: retire three-argument RegExp extensions,
Bug 592027 - YARR: regexp syntax error on unbalanced parens.
"1+2".replace("1+2", "$&+3","g");
"1+2".replace("1+2", "$&+3","i");
etc...
However, there are *no error* in the Error console.
it annoy everybody in developing Web site and Extensions.
Reporter | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
this is of course invalid because "+" is a repetition operator which means you were trying to match "12", "112", etc. and as "1+2" doesn't match that, it wasn't changed.
Resolution: WONTFIX → INVALID
Reporter | ||
Comment 2•14 years ago
|
||
(In reply to comment #1)
> this is of course invalid because "+" is a repetition operator which means you
> were trying to match "12", "112", etc. and as "1+2" doesn't match that, it
> wasn't changed.
Why invalid?
in Firefox 3.6.8, both return "1+2+3".
Resolution: INVALID → WONTFIX
Well, Bug 592143 asked to remove support for flags, that's wontfix.
Thus, a bug expecting flags not to apply in the face of wontfix is invalid.
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/replace
str.replace(regexp|substr, newSubStr|function[, Non-standard flags]);
flags
A string containing any combination of the RegExp flags:
...
example
...
In this version, a string is used as the first parameter and the global and ignore case flags are specified in the flags parameter.
var str = "Apples are round, and apples are juicy.";
var newstr = str.replace("apples", "oranges", "gi");
Resolution: WONTFIX → INVALID
Comment 4•14 years ago
|
||
timeless, why did the behavior change from 3.6.8 here? That seems to not have been intended...
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Updated•14 years ago
|
blocking2.0: --- → ?
Keywords: regression
Assignee | ||
Comment 5•14 years ago
|
||
The bug is valid -- we differ from our former behavior. We used to compile the example as a "flat" regexp. Escaping regexp metachars in the engine can be used as a workaround for this case.
Assignee: general → cdleary
Status: REOPENED → ASSIGNED
Updated•14 years ago
|
blocking2.0: ? → betaN+
Assignee | ||
Comment 6•14 years ago
|
||
Simple workaround until we remove the three-argument form.
Attachment #476081 -
Flags: review?(dmandelin)
Assignee | ||
Comment 7•14 years ago
|
||
Realized I forgot a null check -- which will occur if we OOM due to a flattening. Will fix that with the rest of the review comments.
Comment 8•14 years ago
|
||
The basic approach seems OK, but I wonder if it might not be easier to add a flat-match mode to Yarr.
flattenPattern seems overcomplicated to me--does it really cost us anything to just use the buffer every time? It would make the code easier to read. Also, what is lastCharWasEscape for?
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #8)
> The basic approach seems OK, but I wonder if it might not be easier to add a
> flat-match mode to Yarr.
I'd rather minimize the differences between us and upstream for merging and patch sharing purposes.
> flattenPattern seems overcomplicated to me--does it really cost us anything to
> just use the buffer every time? It would make the code easier to read. Also,
> what is lastCharWasEscape for?
We could use the buffer every time -- this is a rare/slow path anyway. I realized that you didn't actually care whether the last character was an escape, but forgot to remove that declaration!
Assignee | ||
Comment 10•14 years ago
|
||
(In reply to comment #9)
> I'd rather minimize the differences between us and upstream for merging and
> patch sharing purposes.
Especially because we dislike this feature :-) -- seems a bad reason to create code skew.
Assignee | ||
Comment 11•14 years ago
|
||
Simplified!
Attachment #476081 -
Attachment is obsolete: true
Attachment #476362 -
Flags: review?(dvander)
Attachment #476081 -
Flags: review?(dmandelin)
Assignee | ||
Comment 12•14 years ago
|
||
Sorry, previous was stale exported patch.
Attachment #476362 -
Attachment is obsolete: true
Attachment #476369 -
Flags: review?(dvander)
Attachment #476362 -
Flags: review?(dvander)
Assignee | ||
Updated•14 years ago
|
Attachment #476369 -
Flags: review?(dvander) → review?(lw)
Comment 13•14 years ago
|
||
Comment on attachment 476369 [details] [diff] [review]
Flattens metachars in the three-argument replace form.
Just a few nits:
>+ for (jschar *it = patstr->chars(); it != patstr->chars() + patstr->length(); ++it) {
I know its cold code, but for the sake of setting a good example and not setting of the spidey-senses of passer-byers, could you compute a beg/end pair before the loop (since neither chars() nor length() is a trivial accessor)?
>+ if (!cb.append(ESCAPE_CHAR) ||
>+ !cb.append(*it))
Seems like this could fit on one line (thereby avoiding a nit about no {'s around the then stmt ;-)
>+ static const uint32 optarg = 2;
The future says 'thank you' :) Perhaps even a comment about what optarg means?
>+++ b/js/src/trace-test/tests/basic/bug593663-regexp.js
>@@ -0,0 +1,1 @@
>+assertEq( "1+2".replace("1+2", "$&+3","g"), "1+2+3");
Could you write a similar test for every character in isMetaChar and perhaps some extra tests for odd things like \+, \\+, \\\, \\\\ and check that it passes on TM tip and 3.6? The pessimist in me assumes that there is at least one special case in that set and it can't be as simple as just throwing \ in front ;-)
Attachment #476369 -
Flags: review?(lw) → review+
Comment 15•14 years ago
|
||
Seems like this might be important to fix or b7, from dup'age.
/be
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/faa24a902263
(There was a comment about what optarg meant elsewhere in the file.)
Whiteboard: fixed-in-tracemonkey
Comment 17•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 18•14 years ago
|
||
oops, sorry
You need to log in
before you can comment on or make changes to this bug.
Description
•