This is an archive of the discontinued Mercurial Phabricator instance.

rust: core implementation for lazyancestors
ClosedPublic

Authored by gracinet on Dec 15 2018, 6:42 AM.

Details

Summary

Once exposed through appropriate bindings, this
should be able to replace ancestor.lazyancestors
entirely.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Dec 15 2018, 6:42 AM
kevincox accepted this revision.Dec 15 2018, 8:08 AM
kevincox added inline comments.
rust/hg-core/src/ancestors.rs
120

Can you clarify this is_empty to match rust convention?

121

if !self.visit.is_empty()

124

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

193

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.

yuja added a subscriber: yuja.Dec 15 2018, 11:45 PM
  • a/rust/hg-core/src/lib.rs

+++ b/rust/hg-core/src/lib.rs
@@ -2,8 +2,10 @@

This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version.
+use std::clone::Clone;

Nit: it's in prelude.

/// The simplest expression of what we need of Mercurial DAGs.
-pub trait Graph {
+pub trait Graph: Clone {

I think it's the requirement of the caller, i.e. G: Graph + Clone.
Not all graphs have to be clone-able.

Alternatively, maybe the LazyAncestor can be moved to the cpython crate,
where PyClone is available.

+ / Tell if the iterator is about an empty set
+
/
+ / The result does not depend whether the iterator has been consumed
+
/ or not.
+ /// This is mostly meant for iterators backing a lazy ancestors set
+ pub fn empty(&self) -> bool {

Nit: s/empty/is_empty/ because empty() can be a verb.

gracinet marked 4 inline comments as done.Dec 22 2018, 11:24 AM
gracinet updated this revision to Diff 12951.

@yuja, yes a Graph not implementing Clone is already a good thing, as it avoids to implement Clone in hg-direct-ffi prematurely. I think the whole hg::LazyAncestors can end up being useful from core Rust code, too, that's why I prefer that to a hg-cpython only implementation.

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

agreed, and made a clearer version of the last lines as an OR statement using self.seen.is_empty()

gracinet updated this revision to Diff 12954.Dec 22 2018, 11:24 AM
This revision was automatically updated to reflect the committed changes.