Page MenuHomePhabricator

rust-discovery: core implementation for take_quick_sample()

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



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

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
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.

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.


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.


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.