Indexes 1: Change Package and related traits to not return references to fields#1336
Indexes 1: Change Package and related traits to not return references to fields#1336
Conversation
dcookspi
added
SPI AOI
dcookspi
changed the title
dcookspi
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| fn compat(&self) -> Arc<Compat> { | ||
| self.compat.clone() |
There was a problem hiding this comment.
We've discussed this elsewhere already, but for the record the suggestion is to use Cow instead of Arc to provide a way for some implementations to be able to return an owned value without sacrificing some places (like this one) from being able to return a cheap reference instead of cloning.
A perhaps even better alternative would be to change this method to return impl SomeNewTrait where that trait contains methods to access whatever is needed from whatever is currently calling this compat() method. Then implementers of Versioned are free to return anything that conforms to that trait.
There was a problem hiding this comment.
Todo:
- Swap the updated Arc<..> return values out for Cow<..>.
There was a problem hiding this comment.
I've switched the methods over to return Cows, and updated the rest of the PRs in the chain as well.
There was a problem hiding this comment.
Turned the alternative into an issue for discussion: #1346
dcookspi
force-pushed
the
index-1-package-trait-changes
branch
from
2f40d65 to
fb6e0dd
Compare
| /// The expanded list of components selected for this resolved item | ||
| pub fn selected_components(&self) -> BTreeSet<&Component> { | ||
| let mut installed_components: BTreeSet<_> = self.request.pkg.components.iter().collect(); | ||
| pub fn selected_components(&self) -> BTreeSet<Component> { |
There was a problem hiding this comment.
I think there could be a case for using Cow here too to allow this method to continue borrowing and avoid cloning but looking at where this method is used it doesn't seem like it is a critical path for the overhead to matter.
There was a problem hiding this comment.
I wondered about that too, but the method didn't show up in the solve profiling before, or after, the change.
…nces to internal fields. This allows alternate package implementations to use the traits, such as the forthcoming indexed based packages. Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>
dcookspi
force-pushed
the
index-1-package-trait-changes
branch
from
fb6e0dd to
9bd9685
Compare
2 participants
Note: for info on benefits of indexing for spk solves see #1340 (5 of 5). Maybe start there and work back down to this PR if you prefer to review PRs top down.
Changes various Package traits' methods to return Arc's instead of references to internal fields. This allows other package implementations, without matching fields, to use the traits. This is one of the changes that supports adding indexes and index based packages to Spk repositories.
This also adds the
ArcCompatobject as helper for serializing and deserializingArc<Compat>fields.This is 1 of 5 chained PRs for adding indexes to spk solves:
new_unchecked()constructors to spk schema objects #1337spk repo indexsubcommand for index generation and updates #1340