D5416: rust: translation of missingancestors

[kevincox (Kevin Cox)]

kevincox added inline comments.

INLINE COMMENTS

> ancestors.rs:154
> +            _ => true,
> +        }
> +    }

Why not just write:

  self.bases.iter().any(|b| != NULL_REVISION)

It is much clearer what you mean and I suspect the performance is very similar. See the below example. If you want the performance then I prefer `c` which is even a bit faster looking.

https://rust.godbolt.org/z/-JJ61P

> ancestors.rs:161
> +    /// read the bases attribute of a ancestor.missingancestors instance.
> +    pub fn get_bases<'a>(&'a self) -> &'a HashSet<Revision> {
> +        &self.bases

Would it be possible to return a more abstract type such as `Interator<Item=Revision>`? I would also prefer to just enclose this completely as IIUC we are somewhat lazily computing the bases so this function has some unspecified preconditions.

> ancestors.rs:278
> +            let this_visit_is_revs = {
> +                if revs_visit.contains(&curr) {
> +                    missing.push(curr);

use `if revs_visit.remove(&curr)` in the condition to avoid duplication in the code.

> ancestors.rs:447
> +    fn test_missing_bases() {
> +        let mut missanc =
> +            MissingAncestors::new(Stub, [5, 3, 1, 3].iter().cloned());

It tool me a while to get this abbreviation. Maybe it should just be spelt out.

> ancestors.rs:448
> +        let mut missanc =
> +            MissingAncestors::new(Stub, [5, 3, 1, 3].iter().cloned());
> +        let mut as_vec: Vec<Revision> =

I'm surprised that the `.iter().cloned()` is required with the argument being an `impl IntoIterator`

REPOSITORY
  rHG Mercurial

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

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