kv: remove Clone requirement for iterators, declutter lifetimes#209
Open
npry wants to merge 2 commits into
Open
kv: remove Clone requirement for iterators, declutter lifetimes#209npry wants to merge 2 commits into
Clone requirement for iterators, declutter lifetimes#209npry wants to merge 2 commits into
Conversation
Signed-off-by: Nathan Perry <nathan@tailscale.com> Change-Id: I929c6926967b439649f78a1b46cb1cc06a6a6964
Clone requirement for iterators, declutter lifetimes
Signed-off-by: Nathan Perry <nathan@tailscale.com> Change-Id: I2c1b0aeb4941cc12038b3e2bea8719c06a6a6964
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #208
Nerd-sniped me on the iterator requiring
Clone(first commit), but in the course of that, I thought that the API (mostly inoperations) could be simpler if the traits used associated types more rather than type params (second commit).The first commit does the
Clonebound on the iterator by doing much more exhaustive lifetime accounting — it separately tracks the'store,'guard,'tx, and'r(ref-to-txn-or-guard) lifetimes and imposes a strict containment in that order everywhere. This is overkill — the second commit rolls it back because the API changes obviate the need — but that lets the inner iterator be held for a much more specific lifetime and avoids the need toClone.The second commit is an observation mainly that you can get the
Storage(and index base table) from the{Table,Index}Descrather than passing it through everywhere in parallel by adjusting how the associated types work a bit and writing a trait to wrapStorage. There's a bit of a sacrifice in verbosity in getting some ATs back out (a lot of<Desc as TableDesc>::Key), but this removes the need for the'alifetime nearly everywhere and makesschema::GeneratedStoragemostly implicit. Most of this is just tracking that through and being more surgical with the lifetimes