Page MenuHomePhabricator

rust-matchers: add function to generate a regex matcher function
Needs ReviewPublic

Authored by Alphare on Jan 17 2020, 5:41 AM.


Group Reviewers

This function will be used to help build the upcoming IncludeMatcher. While
Re2 is still used and behind a feature flag, this function returns an error
meant for fallback in the default case.

Diff Detail

rHG Mercurial
No Linters Available
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 17 2020, 5:41 AM
pulkit accepted this revision.Fri, Feb 7, 4:12 PM
This revision is now accepted and ready to land.Fri, Feb 7, 4:12 PM
pulkit added a comment.Fri, Feb 7, 4:47 PM

This one failed to apply on latest default, needs rebase.

martinvonz requested changes to this revision.Mon, Feb 10, 7:48 PM
martinvonz added a subscriber: martinvonz.
martinvonz added inline comments.

Hmm, I don't like to replicate this into Rust. I argued for a long time with Boris over a year ago that we should see if we can remove it from Python. He said they (Octobus, I think) would look into that if I would just queue the workaround for the time being. Could you see if you can simplify the Python code first instead?

See for discussion.

This revision now requires changes to proceed.Mon, Feb 10, 7:48 PM
Alphare added inline comments.Tue, Feb 11, 5:09 AM

I am having a little trouble reading the patchwork thread, but I gather that you want to get rid of the preventive splitting of patterns and just let the regex engine handle it on its own? Was this measure taken because of a bug in Python's re or because its exceptions were too coarse/unusable?
I'll look into the behavior of Re2, in that case.

Alphare updated this revision to Diff 20154.Tue, Feb 11, 5:54 AM
Alphare added inline comments.Tue, Feb 11, 5:58 AM

I've looked into the behavior of Re2. It will return an error if the DFA runs out of memory, which seems perfectly reasonable.
I will simplify the Rust code, however I feel like you're better suited than I am to fix the Python side of things, since I don't really understand the ins-and-outs of the problem.

Alphare updated this revision to Diff 20160.Tue, Feb 11, 6:13 AM