Page MenuHomePhabricator

rust-nodemap: NodeMap trait with simplest implementation
ClosedPublic

Authored by gracinet on Jan 6 2020, 2:25 PM.

Details

Summary

We're defining here only a small part of the immutable
methods it will have at the end. This is so we can
focus in the following changesets on the needed abstractions
for a mutable append-only serializable version.

The first implementor exposes the actual lookup
algorithm in its simplest form. It will have to be expanded
to account for the missing methods, and the special cases
related to NULL_NODE.

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.Jan 6 2020, 2:25 PM
gracinet updated this revision to Diff 19134.Jan 10 2020, 10:03 AM
gracinet updated this revision to Diff 19324.Jan 15 2020, 6:47 PM
kevincox accepted this revision.Jan 16 2020, 5:08 AM
kevincox added inline comments.
rust/hg-core/src/revlog/nodemap.rs
39

Can you please add doc-comments for this? I find that documenting trait methods is especially important.

250

I don't think the <'_> is necessary.

martinvonz added inline comments.
rust/hg-core/src/revlog/nodemap.rs
195

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.

195

Do we need Send? Maybe it later when you need it (unless you actually need it now and I'm just mistaken, of course).

198

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?

Alphare added inline comments.
rust/hg-core/src/revlog/nodemap.rs
195

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?

Alas, trait aliases are not stable: rust#55628.

On a more subjective note, I find this syntax quite readable as it is (as readable as Rust usually is), and since this is a single occurrence in a private field, I find it reasonable.

martinvonz added inline comments.Jan 21 2020, 5:27 PM
rust/hg-core/src/revlog/nodemap.rs
195

Alas, trait aliases are not stable: rust#55628.

I didn't know about trait aliases, but I think you can already do this:

trait Thing : Deref<Target = [Block]> + Send  {}

... since this is a single occurrence in a private field, I find it reasonable.

It's one instance right now. It's 7 instances of Box<dyn Deref<...>> at the end of the series...

Alphare added inline comments.Jan 22 2020, 4:43 AM
rust/hg-core/src/revlog/nodemap.rs
195

I didn't know about trait aliases, but I think you can already do this:
trait Thing : Deref<Target = [Block]> + Send {}

Using an empty supertrait will not work because fat pointers have different metadata.
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=4f1d374e426c9791cbf73421160dc7c3

It's one instance right now. It's 7 instances of Box<dyn Deref<...>> at the end of the series...

Ah true, that is indeed verbose, but manageable. Unfortunately, from a language perspective, I think that a type macro would be our only choice, but my opinion is that 1) it's ugly and more code to maintain and 2) I'm not even sure it would work.

gracinet added inline comments.Jan 22 2020, 10:10 AM
rust/hg-core/src/revlog/nodemap.rs
195

I've looked into trait alias while working on this, for the exact same reason, and went through most you guys are saying about that.

Seems that this is complicated for the language because they'd like to have the
possibility to impl it.

195

About Send, without it we can't expose NodeTree to the Python layer, IIRC

martinvonz added inline comments.Jan 22 2020, 11:23 AM
rust/hg-core/src/revlog/nodemap.rs
195

How about something like this then?

type BlockSource = Box<dyn Deref<Target = [Block]> + Send>;
type ByteSource = Box<dyn Deref<Target = [u8]> + Send>;

I won't insist, so up to you if you think it helps.

gracinet retitled this revision from rust-nodemap: NodeMap trait with simplest implementor to rust-nodemap: NodeMap trait with simplest implementation.Jan 27 2020, 10:49 AM
gracinet updated this revision to Diff 19631.
gracinet marked 3 inline comments as done.Jan 27 2020, 10:53 AM
gracinet added inline comments.
rust/hg-core/src/revlog/nodemap.rs
39

Sure, indeed it's more important than with the impl.

195

Missed the type (not trait) alias suggestion. Maybe for the next update, then

198

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.

250

Removed

martinvonz added inline comments.Jan 27 2020, 1:40 PM
rust/hg-core/src/revlog/nodemap.rs
76

can the explicit lifetime be dropped?

198

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

199

can the explicit lifetime be dropped?

219

can the explicit lifetime be dropped?

250

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

256

can the explicit lifetime be dropped?

gracinet marked 2 inline comments as done.Jan 27 2020, 2:13 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.