Page MenuHomePhabricator

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

Authored by gracinet on Dec 12 2018, 10:11 AM.

Details

Summary

This will allow for simple iteration on parent revisions,
such as:

for parent in graph.parents(rev)?.iter().cloned()

This seems to be a zero overhead abstraction once built in
release mode.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gracinet created this revision.Dec 12 2018, 10:11 AM

This is a nice improvement. It still seems odd to me to return a list of two parents when there could be 0, 1 or 2 parents. I guess this is for performance reasons?

rust/hg-core/src/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);
}
118
let [p1, p2] = ...
gracinet updated this revision to Diff 12831.Dec 12 2018, 11:26 AM
gracinet marked 2 inline comments as done.Dec 12 2018, 11:37 AM
gracinet added a subscriber: yuja.

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 D5370, we could make a utility function to filter out NULL_REVISION later on. Or (why not) an iterator that does all needed sanitizing.

rust/hg-core/src/ancestors.rs
62

I like the looping.

118

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

gracinet marked 2 inline comments as done.Dec 13 2018, 7:33 AM
This revision was automatically updated to reflect the committed changes.
yuja added a comment.Dec 13 2018, 7:36 AM

Queued the first two, thanks.

This is a nice improvement. It still seems odd to me to return a list of two parents when there could be 0, 1 or 2 parents. I guess this is for performance reasons?

It's a direct mapping from the revlog storage. I think [Revision; 2]
is good enough at this layer as I see the Graph trait is actually an
abstraction for storage-level objects.

yuja added a comment.Dec 13 2018, 7:36 AM

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.

Curious. Can you point out which? I think many things would be broken if
p1 is null but p2 isn't.