Skip to content

Reset Regex state after calling 'test', resolves #7 #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

sharkdp
Copy link
Contributor

@sharkdp sharkdp commented Jan 9, 2016

Previously, the second assert in the following code would fail:

let pattern = regex "a" (parseFlags "g")
assert $ test pattern "a"
assert $ test pattern "a"

The reason is that the RegExp object in JavaScript maintains a global state (for details, see the discussion in #7).

This commit resets the lastIndex property on the RegExp object, such that both calls to test return the same result.

@@ -29,7 +29,9 @@ exports.flags = function (r) {

exports.test = function (r) {
return function (s) {
return r.test(s);
var result = r.test(s);
r.lastIndex = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we restore the previous value of r.lastIndex instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Purescript Regex API, there is no other function that affects lastIndex (if I'm correct), so I thought resetting to 0 would be fine. But your solution is the safer alternative (there might be foreign code which affects the lastIndex property). Thanks.

@paf31
Copy link
Contributor

paf31 commented Jan 10, 2016

It's a shame it works this way, but I can see why it might be useful in code where performance is a must. Perhaps we should find a way to represent things like test using Eff in the case where the global flag is set.

@davidchambers
Copy link
Contributor

LGTM

@sharkdp
Copy link
Contributor Author

sharkdp commented Jan 10, 2016

It's a shame it works this way, but I can see why it might be useful in code where performance is a must. Perhaps we should find a way to represent things like test using Eff in the case where the global flag is set.

I agree that - with these changes - the global flag in a Regex is useless when using test. The question is: when would I be interested in using (an effectful version of) test? I actually cannot think of any reasonable use case apart from counting the number of matches [*]. However, to get the number of matches, I would rather use match directly, which gives me all results. But I might be missing something here.

I don't really have a strong opinion on this, but using Eff just to model this (arguably) weird behavior of the JavaScript RegExp API seems strange to me. An alternative might be to provide a version of test like this:

testFromIndex :: Regex -> Int -> String -> { match :: Boolean, newIndex :: Int }

which could be used in a fold/loop.

[*] Note that test only gives me a Boolean whether or not the Regex matched. Otherwise it might be interesting to use an effectful version of test in a loop and exit early.

@garyb
Copy link
Member

garyb commented Mar 24, 2016

Could you rebase this one please?

@sharkdp sharkdp force-pushed the fix-regex-global-state branch from ac55b2b to f5d9f35 Compare March 24, 2016 20:48
Previously, the second `assert` in the following code would fail:
``` purs
let pattern = regex "a" (parseFlags "g")
assert $ test pattern "a"
assert $ test pattern "a"
```
because the RegExp object in JavaScript maintains a global state
(for details, see the discussion in purescript#7).

This commit resets the `lastIndex` property on the RegExp object,
such that both calls to `test` return the same result.
@sharkdp sharkdp force-pushed the fix-regex-global-state branch from f5d9f35 to 26941a9 Compare March 24, 2016 20:50
@garyb
Copy link
Member

garyb commented Mar 24, 2016

Thanks!

@garyb garyb merged commit e4317a2 into purescript:master Mar 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants