This is an archive of the discontinued Mercurial Phabricator instance.

rust-discovery: core implementation for take_quick_sample()
ClosedPublic

Authored by gracinet on Apr 16 2019, 5:51 PM.

Details

Summary

This makes in particular rand no longer a testing dependency.

We keep a seedable random generator on the PartialDiscovery object
itself, to avoid lengthy initialization.

In take_quick_sample() itself, we had to avoid keeping the reference
to self.undecided to cope with the mutable reference introduced
by the the call to limit_sample, but it's still manageable without
resorting to inner mutability.

Sampling being prone to be improved in the mid-term future, testing
is minimal, amounting to checking which code path got executed.

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.Apr 16 2019, 5:51 PM
kevincox accepted this revision.Apr 20 2019, 2:52 PM
kevincox added inline comments.
rust/hg-core/src/discovery.rs
70

You can do

if see.insert(current).is_some() {
  continue; // Already present.
}

And avoid the insert below. Alternatively use the entry API which may be a bit more readable.

87

You can pattern match away the reference since Revision is Copy.

for &p in parentsfn(curent)? {
  // ...
}
gracinet marked 2 inline comments as done.May 6 2019, 11:30 AM
gracinet updated this revision to Diff 15021.

@kevincox thanks for the initial review, I think I've implemented the corrections you suggested.

rust/hg-core/src/discovery.rs
70

Ah yes that's right, let this one through.

Since seen is not used in the loop after that check (there's an early return that doesn't matter in this case), I found the boolean return of HashSet::insert() does the job neatly.

This revision was automatically updated to reflect the committed changes.