( )⚙ D7910 rust-re2: add wrapper for calling Re2 from Rust

This is an archive of the discontinued Mercurial Phabricator instance.

rust-re2: add wrapper for calling Re2 from Rust
ClosedPublic

Authored by Alphare on Jan 16 2020, 8:54 AM.

Details

Summary

This assumes that Re2 is installed following Google's guide. I am not sure
how we want to integrate it in the project, but I think a follow-up patch would
be more appropriate for such work.
As it stands, *not* having Re2 installed results in a compilation error, which
is a problem as it breaks install compatibility. Hence, this is gated behind
a non-default with-re2 compilation feature.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Jan 16 2020, 8:54 AM
kevincox accepted this revision.Jan 16 2020, 10:41 AM

Does mercurial currently use RE2? If not then can you explain why we don't just use the rust regex crate?

kevincox requested changes to this revision.Jan 16 2020, 10:42 AM
kevincox added inline comments.
rust/hg-core/src/re2/rust_re2.cpp
23

This is never freed. Should we add a Drop implementation in rust?

This revision now requires changes to proceed.Jan 16 2020, 10:42 AM

Does mercurial currently use RE2? If not then can you explain why we don't just use the rust regex crate?

We have support for using re2 iff the correct Python bindings are installed. That said, I thought the rust regex crate would be compatible with re2? If we can't use that, we should document why so that future hackers (like me) don't try and simplify....

While I think the rust regex crate is heavily inspired by RE2 and uses a fairly compatible syntax I don't think there is a goal to be 100% compatible and I would be surprised if there weren't any differences. What I am trying to say is that if we are going to change engines we may as well change to regex. If we are using RE2 for backwards compatibility then this change makes sense.

Alphare marked an inline comment as done.Jan 16 2020, 4:50 PM
Alphare updated this revision to Diff 19386.
kevincox accepted this revision.Jan 17 2020, 10:08 AM
Alphare updated this revision to Diff 19546.Jan 24 2020, 4:57 AM
Alphare updated this revision to Diff 19938.Feb 6 2020, 10:14 AM
Alphare updated this revision to Diff 20040.Feb 10 2020, 10:46 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.