D5440: rust: core implementation for lazyancestors

[kevincox (Kevin Cox)]

kevincox accepted this revision.
kevincox added inline comments.

INLINE COMMENTS

> ancestors.rs:120
> +    /// This is mostly meant for iterators backing a lazy ancestors set
> +    pub fn empty(&self) -> bool {
> +        if self.visit.len() > 0 {

Can you clarify this `is_empty` to match rust convention?

> ancestors.rs:121
> +    pub fn empty(&self) -> bool {
> +        if self.visit.len() > 0 {
> +            return false;

`if !self.visit.is_empty()`

> ancestors.rs:124
> +        }
> +        let seen = self.seen.len();
> +        if seen > 1 {

I think this variable makes the code harder to read. I would just repeat the calls to `.len()`.

> ancestors.rs:196
> +
> +    #[inline]
> +    pub fn contains(&mut self, rev: Revision) -> Result<bool, GraphError> {

In general I wouldn't add these without careful benchmarking. In this case the compiler can trivially notice that this method is a good inlining candidate.

REPOSITORY
  rHG Mercurial

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

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