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
Lint Skipped
Unit
Unit Tests Skipped

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().

196

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.