D5370: rust: core implementation of missingancestors (no bindings)

gracinet (Georges Racinet) phabricator at mercurial-scm.org
Wed Dec 12 10:11:13 EST 2018


gracinet marked 11 inline comments as done.
gracinet added a comment.


  @yuja trying to submit the v2 with phabsend instead of arcanist… hope it'll be linked properly

INLINE COMMENTS

> kevincox wrote in ancestors.rs:201
> Isn't this loop redundant with the retain call above?

indeed, must have been a leftover

> kevincox wrote in ancestors.rs:235
> I would prefer `let (p0, p1) = ...`. This makes it obvious exactly what data you are getting.

In the next version, I'm simply doing a for loop on `.iter().cloned()`, it's even better imho.

> kevincox wrote in lib.rs:26
> You can add `.cloned()` to remove the references. I would be surprised if this has poor code generation.
> 
> As for HashSet vs bit array I would stick to HashSet for now. HashSet also has the advantage of being sparse. I guess after the initial implementation we could benchmark the two to see what is better.

Thanks for the tip with `.cloned()` that's exactly what I was missing.

Out of curiosity, I took a look at the generated optimized assembly code: zero cost abstraction, indeed.

About HashSet, yes that's exactly what I'm thinking.

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list