Skip to content

Add Regex::searchAllMatches method, remove searchAll #2394

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

Closed
wants to merge 1 commit into from

Conversation

WGH-
Copy link
Contributor

@WGH- WGH- commented Sep 5, 2020

searchAll interface was pretty strange, as it returned a reversed flattened list of all groups of all matches. For example, given the regexp "([a-z])([0-9])", and the string "a1b2", it would return the list ["2", "b", "b2", "1", "a", "a1"].

searchAllMatches introduces better interface. In the aforementioned scenario it would return [["a1", "a", "1"], ["b2", "b", "2"]]. Also, unlike searchAll, it doesn't make string copies itself, and returns just offsets, like searchOneMatch does.

searchAll interface was pretty strange, as it returned a reversed
flattened list of all groups of all matches. For example, given the
regexp "([a-z])([0-9])", and the string "a1b2", it would return the list
["2", "b", "b2", "1", "a", "a1"].

searchAllMatches introduces better interface. In the aforementioned
scenario it would return [["a1", "a", "1"], ["b2", "b", "2"]]. Also,
unlike searchAll, it doesn't make string copies itself, and returns just
offsets, like searchOneMatch does.
@zimmerle
Copy link
Contributor

zimmerle commented Sep 8, 2020

Thanks @WGH-. @martinhsv : ping. That is somewhat in the same piece of code that you have worked with.

@martinhsv
Copy link
Contributor

I haven't reviewed this in detail, but it looks like it at least has the same correctness problem that I outlined in #2393 (that it always immediately advances the offset after finding an empty string as a full match).

Note my other remarks in that pull request as well. To avoid too much implementation of overlapping functionality, it might be simplest to allow the rxGlobal + searchGlobal to proceed (assuming it passes muster), and then somewhat later look to migrate remaining users of searchAll to use searchGlobal.

@zimmerle
Copy link
Contributor

Hi @WGH- I am closing this and #2393 in favor of #2396. Please let me know if I am missing something.

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.

3 participants