D5415: rust: changed Graph.parents to return [Revision; 2]

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


gracinet marked 2 inline comments as done.
gracinet added a subscriber: yuja.
gracinet added a comment.


  Thanks ! About the fact to always returning two elements, I've been hesitating about that. This is what the C function we're wrapping provides anyway, so you could say it's for simplicity. I don't take any risks either wrt performance. But it's true it would be also simpler for callers not to worry about that at all. Another point is that there is no firm guarantee that a valid revision occurs before `NULL_REVISION`. I've found examples in `revlog.c` where the second parent is (seemlingly on purpose) not ignored if the first is `NULL_REVISION`, but I wouldn't want at this point to impose it from the `parents()` method right away.
  
  As @yuja said on https://phab.mercurial-scm.org/D5370, we could make a utility function to filter out `NULL_REVISION` later on. Or (why not) an iterator that does all needed sanitizing.

INLINE COMMENTS

> kevincox wrote in ancestors.rs:62
> I would do `let [p1, p2]`  still as it shows the reader that you handled all the nodes. Alternatively loop over them all.
> 
>   for parent in this.graph.parents(rev)? {
>     this.conditionally_push_rev(parent);
>   }

I like the looping.

> kevincox wrote in ancestors.rs:118
>   let [p1, p2] = ...

oh, great, didn't know it would work, should have tried !

REPOSITORY
  rHG Mercurial

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

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


More information about the Mercurial-devel mailing list