D6260: rust-discovery: takefullsample() core implementation

[kevincox (Kevin Cox)]

kevincox accepted this revision.
kevincox added inline comments.

INLINE COMMENTS

> discovery.rs:108
> +    cur: usize,
> +}
> +

You might also consider an enum

  enum Parents {
    Zero,
    One(Revision),
    Two(Revision, Revision),
  }

This also makes the iterator implementation a little clearer.

> discovery.rs:137
> +                return self.next();
> +            }
> +        }

Can't you just call `self.next()` in both cases?

> discovery.rs:280
> +        let mut children: HashMap<Revision, Vec<Revision>> = HashMap::new();
> +        for rev in self.undecided.as_ref().unwrap().iter().cloned() {
> +            for p in self.graph.parents(rev)?.iter().cloned() {

Generally I would just do this.

  for &rev in self.undecided.as_ref().unwrap() {

> discovery.rs:281
> +        for rev in self.undecided.as_ref().unwrap().iter().cloned() {
> +            for p in self.graph.parents(rev)?.iter().cloned() {
> +                if p != NULL_REVISION {

Same.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6260

To: gracinet, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel