D5416: rust: translation of missingancestors

[yuja (Yuya Nishihara)]

yuja added a comment.


  Queued this, thanks.
  
  > +    /// Add rev's parents to self.bases
  >  +    #[inline]
  >  +    fn add_parents(&mut self, rev: Revision) -> Result<(), GraphError> {
  >  +        // No need to bother the set with inserting NULL_REVISION over and
  >  +        // over
  >  +        for p in self.graph.parents(rev)?.iter().cloned() {
  >  +            if p != NULL_REVISION {
  >  +                self.bases.insert(p);
  >  +            }
  >  +        }
  >  +        Ok(())
  >  +    }
  
  The comment sounds like we don't care the NULL_REVISION (i.e. the bases could
  be extend()-ed), but we do explicitly avoid doing that.
  
  > +        let mut missing: Vec<Revision> = Vec::new();
  >  +        for curr in (0..=start).map(|i| start - i) {
  
  Nit: can be `(0..=start).rev()` ?
  
  > +            if revs_visit.is_empty() {
  >  +                break;
  >  +            }
  >  +            if both_visit.contains(&curr) {
  >  +                // curr's parents might have made it into revs_visit through
  >  +                // another path
  >  +                // TODO optim: Rust's HashSet.remove returns a boolean telling
  >  +                // if it happened. This will spare us one set lookup
  >  +                both_visit.remove(&curr);
  >  +                for p in self.graph.parents(curr)?.iter().cloned() {
  >  +                    if p == NULL_REVISION {
  >  +                        continue;
  >  +                    }
  >  +                    revs_visit.remove(&p);
  >  +                    bases_visit.insert(p);
  >  +                    both_visit.insert(p);
  >  +                }
  >  +                continue;
  >  +            }
  >  +            // in Rust, one can't just use mutable variables assignation
  >  +            // to be more straightforward. Instead of Python's
  >  +            // thisvisit and othervisit, we'll differentiate with a boolean
  >  +            let this_visit_is_revs = {
  >  +                if revs_visit.remove(&curr) {
  >  +                    missing.push(curr);
  >  +                    true
  >  +                } else if bases_visit.contains(&curr) {
  >  +                    false
  >  +                } else {
  >  +                    // not an ancestor of revs or bases: ignore
  >  +                    continue;
  >  +                }
  >  +            };
  
  Maybe this could be extracted to a function
  `visit_parents(&mut revs_visit, &mut bases_visit, select_this, select_other)`
  where `select_this` and `select_other` are `FnMut(&mut, &mut) -> &mut`, but
  I don't know if that will improve the readability.

REPOSITORY
  rHG Mercurial

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

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