This is an archive of the discontinued Mercurial Phabricator instance.

rust-nodemap: insert method
ClosedPublic

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

Details

Summary

In this implementation, we are in direct competition
with the C version: this Rust version will have a clear
startup advantage because it will read the data from disk,
but the insertion happens all in memory for both.

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

gracinet created this revision.Jan 6 2020, 2:25 PM
gracinet updated this revision to Diff 19328.Jan 15 2020, 6:47 PM
kevincox accepted this revision.Jan 16 2020, 6:01 AM
kevincox added inline comments.
rust/hg-core/src/revlog/nodemap.rs
318

It looks like this return value could use some abstraction but maybe its best to wait until there are more users?

323

I don't quite understand. You have the immutable copy and the mutable copy but that is WAI right?

356

nybble is very vague. Is this deepest_nibble or something?

573

Should these be #[cfg(test)]? We can always remove it later if there is a valid production use but it makes new users think twice.

gracinet updated this revision to Diff 19635.Jan 27 2020, 10:49 AM
marmoute accepted this revision.Feb 7 2020, 2:13 PM
marmoute added a subscriber: marmoute.

The change seesm good to me. However kevincox proposal to use #[cfg(test)] for pad_node seems worth applying inflight.

rust/hg-core/src/revlog/nodemap.rs
318

In pratice this function should have very few user, and they will all be internal. So I am not sure we need something more advanced.

356

This seems to have been updated because the code no longer mention nible on that line.

573

This seems like a good idea.

marmoute updated this revision to Diff 20024.Feb 8 2020, 12:44 PM

The change seesm good to me. However kevincox proposal to use #[cfg(test)] for pad_node seems worth applying inflight.

For future reference, I'm going to add it here, but if you're going to the trouble of uploading a new revision, it would have been nice to do this and save reviewer toil.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

The change seesm good to me. However kevincox proposal to use #[cfg(test)] for pad_node seems worth applying inflight.

For future reference, I'm going to add it here, but if you're going to the trouble of uploading a new revision, it would have been nice to do this and save reviewer toil.

I juste rebased the revision on a more modern tip. I did not actually touched anything inside it.