Page MenuHomePhabricator

rust-dirstate-status: add `walk_explicit` implementation, use `Matcher` trait
ClosedPublic

Authored by Alphare on Nov 29 2019, 1:22 PM.

Details

Summary

This is the first time we actually use the Matcher trait, still for a small
subset of all matchers defined in Python.

While I haven't yet actually measured the performance of this, I have tried
to avoid any unnecessary allocations. This forces the use of heavy lifetimes
annotations which I am not sure we can simplify, although I would be happy
to be proven wrong.

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.Nov 29 2019, 1:22 PM
kevincox requested changes to this revision.Dec 2 2019, 8:26 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/status.rs
125

It seems to me that you can just use one lifetime here since the two input limit the output.

https://rust.godbolt.org/z/57bf6T But maybe I over-simplified the problem.

168

Isn't this the same as the first case? Can we just remove the first case?

240

It surprises me this annotation is required? I thought this would be the assumed meaning.

This revision now requires changes to proceed.Dec 2 2019, 8:26 AM
Alphare updated this revision to Diff 18423.Dec 2 2019, 9:20 AM
Alphare added inline comments.Dec 2 2019, 9:21 AM
rust/hg-core/src/dirstate/status.rs
125

You're right, the complex annotations are only needed on status, this only returns values from files.

168

The if will be needed in a (semi-distant) future series, I'll remove it for now, thanks.

240

I thought so too, but it seems that the compiler's elision rules do not apply with Iterators. It's probable a more complicated problem to solve in a deterministic fashion than just a plain Vec.

kevincox accepted this revision.Dec 2 2019, 10:16 AM

This no longer applies cleanly (and doesn't build when applied with fuzz)

Alphare updated this revision to Diff 18635.Dec 12 2019, 10:40 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.