D7791: rust-nodemap: NodeMap trait with simplest implementation

[martinvonz (Martin von Zweigbergk)]

martinvonz added inline comments.

INLINE COMMENTS

> gracinet wrote in nodemap.rs:153
> Perhaps a better name would be better than this `has_` that indeed feels boolean?
> 
> `check_prefix`?  `confirm` ?
> 
> Previous naming was `validate_candidate`, but that very same name is used at the end of the series when it becomes involved with `NULL_NODE` etc. Then this `has_prefix_or_none` becomes one step in the final verification.
> 
> Returning a `bool` would mean adding a closure to the callers. making it less obvious that they are just chaining the maturation of the response.
> 
> I'm leaving unchanged for now, not sure what the best is. Still note that this is a private function, there's no risk an external caller would be confused by what it does.

Yes, `validate_candidate` sounds much better. Could you send a follow-up with a better name for it?

> gracinet wrote in nodemap.rs:205
> Removed

Still there? Or maybe it's a different one, but I think you can remove the `<'_>` from the `fm::Formatter<'_>`.

> nodemap.rs:76
> +    /// error is returned.
> +    fn find_bin<'a>(
> +        &self,

can the explicit lifetime be dropped?

> nodemap.rs:199
> +/// Return `None` unless the `Node` for `rev` has given prefix in `index`.
> +fn has_prefix_or_none<'p>(
> +    idx: &impl RevlogIndex,

can the explicit lifetime be dropped?

> nodemap.rs:219
> +    /// This partial implementation lacks special cases for NULL_REVISION
> +    fn lookup<'p>(
> +        &self,

can the explicit lifetime be dropped?

> nodemap.rs:256
> +impl NodeMap for NodeTree {
> +    fn find_bin<'a>(
> +        &self,

can the explicit lifetime be dropped?

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: Alphare, martinvonz, durin42, kevincox, mercurial-devel