gracinet abandoned this revision.
gracinet marked an inline comment as done.
gracinet added a comment.
This Differential has been superseded by https://phab.mercurial-scm.org/D5414
through https://phab.mercurial-scm.org/D5417
REPOSITORY
rHG Mercurial
REVISION DETAIL
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
yuja added a comment.
> @yuja: do you mean one of those Differential Revisions of this system for
each commit, sure I can do.
I don't know how Phabricator is working, but probably yes. I meant we want
https://phab.mercurial-scm.org/D5370, https://phab.mercurial-scm.org/D5371,
and
> @yuja: do you mean one of those Differential Revisions of this system for
> each commit, sure I can do.
I don't know how Phabricator is working, but probably yes. I meant we want
D5370, D5371, and D5372 instead of a folded D5370. I think phabsend will
do that way.
> With respect to
gracinet added a comment.
@yuja: do you mean one of those Differential Revisions of this system for
each commit, sure I can do.
With respect to rust-cpython bindings, I'm currently waiting for feedback on
https://github.com/dgrunwald/rust-cpython/issues/164
Perhaps you'd have an idea
yuja added a comment.
Quickly scanned, and looks generally good to me.
> rust: iterator version of Graph.parents
> rust: translation of missingancestors
> rust: translated random test of missingancestors
Can you send these as separate patches?
> An alternative would
Quickly scanned, and looks generally good to me.
> rust: iterator version of Graph.parents
> rust: translation of missingancestors
> rust: translated random test of missingancestors
Can you send these as separate patches?
> An alternative would have been to expose to Python
>
kevincox added a comment.
I think sticking close to the python version makes sense for the initial
version. Then improvements can be made in follow-ups.
INLINE COMMENTS
> gracinet wrote in lib.rs:26
> The reason I've preferred to implement it directly is that `into_iter()`
> iterates on
gracinet added a comment.
I'll include your remark in the upcoming v2, thanks again !
INLINE COMMENTS
> kevincox wrote in ancestors.rs:159
> Is always having an item in the set actually saving many corner cases? It
> seems like you usually check for empty sets anyways.
Not sure about this,
kevincox added a comment.
Overall looks good to me. Just a couple of points.
- Using random numbers for tests without a seed that is logged will create
failures which are basically impossible to reproduce.
- A lot of the comments are comparing to the python, I don't know how useful
gracinet created this revision.
Herald added subscribers: mercurial-devel, mjpieters, kevincox, durin42.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
rust: iterator version of Graph.parents
This is handy for callers that want to simply do:
for p in graph.parents_iter(rev)
11 matches
Mail list logo
