This is an archive of the discontinued Mercurial Phabricator instance.

rust-nodemap: generic NodeTreeVisitor
ClosedPublic

Authored by gracinet on Jan 6 2020, 2:25 PM.

Details

Summary

This iterator will help avoid code duplication when we'll
implement insert(), in which we will need to
traverse the node tree, and to remember the visited blocks.

The structured iterator item will allow different usages from
lookup() and the upcoming insert().

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

gracinet created this revision.Jan 6 2020, 2:25 PM
gracinet updated this revision to Diff 19327.Jan 15 2020, 6:47 PM
kevincox requested changes to this revision.Jan 16 2020, 5:16 AM
kevincox added inline comments.
rust/hg-core/src/revlog/nodemap.rs
254

I would prefer making this a struct so that you can name the fields. You can still pattern match the fields if you want to.

This revision now requires changes to proceed.Jan 16 2020, 5:16 AM
martinvonz added inline comments.
rust/hg-core/src/revlog/nodemap.rs
264–269

There will only be a valid result if this is a leaf, so it might be better to combine leaf and opt into one Option<Option<Revision>> so the caller cannot mistake a opt=None for "missing" when leaf=false.

gracinet edited the summary of this revision. (Show Details)Jan 27 2020, 10:49 AM
gracinet updated this revision to Diff 19634.
gracinet marked 2 inline comments as done.Jan 27 2020, 10:54 AM
gracinet added inline comments.
rust/hg-core/src/revlog/nodemap.rs
264–269

I understand your concern, but <Option<Option<Revision>> would defeat the purpose of not having a dead match arm to fill with a "can't happen" comment in the future insert().

Fortunately, @kevincox suggestion of having NodeTreeVisitor emit a struct provides us with the best of both worlds, since it can have methods.

It should be fully transparent performance-wise, I just hope that is really true.

martinvonz accepted this revision.Jan 27 2020, 2:00 PM
martinvonz added inline comments.
rust/hg-core/src/revlog/nodemap.rs
248

would something like block_index be clearer?

260

can these lifetimes be anonymous?

gracinet marked 2 inline comments as done.Jan 27 2020, 2:09 PM
gracinet added inline comments.
rust/hg-core/src/revlog/nodemap.rs
248

I found it to be clearer for the emitter, but in the iterator implementation, it expresses what's to be done next, same as in many cases I've seeen in Mercurial

gracinet marked an inline comment as done.Jan 27 2020, 2:13 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.