D7791: rust-nodemap: NodeMap trait with simplest implementor
martinvonz (Martin von Zweigbergk)
phabricator at mercurial-scm.org
Tue Jan 21 16:42:57 EST 2020
martinvonz added inline comments.
INLINE COMMENTS
> nodemap.rs:150
> +pub struct NodeTree {
> + readonly: Box<dyn Deref<Target = [Block]> + Send>,
> +}
I find `Deref<Target = [Block]> + Send` hard to understand. I think it would be helpful to name it. Could we at least define an alias for it?
Perhaps it's even better to define a trait for it and add named methods on that, but if those methods would just be `len()` and `index()` it probably doesn't make any sense to define the trait.
> nodemap.rs:150
> +pub struct NodeTree {
> + readonly: Box<dyn Deref<Target = [Block]> + Send>,
> +}
Do we need `Send`? Maybe it later when you need it (unless you actually need it now and I'm just mistaken, of course).
> nodemap.rs:153
> +
> +/// Return `None` unless the `Node` for `rev` has given prefix in `index`.
> +fn has_prefix_or_none<'p>(
I understand that you picked this interface because it works well for the caller, but it feel a little weird to always return `None` or `Some(rev)` where `rev` is one of the function inputs. It's practically a boolean-valued function. Do the callers get much harder to read if you actually make it boolean-valued?
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7791/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7791
To: gracinet, #hg-reviewers, kevincox
Cc: martinvonz, durin42, kevincox, mercurial-devel
More information about the Mercurial-devel
mailing list