Burn DataMarker::Cart down with fire #1279
Conversation
Manishearth
requested review from
gregtatum,
nordzilla,
sffc,
zbraniecki and
a team
as code owners
provider/core/src/marker/impls.rs
Outdated
| @@ -26,7 +24,6 @@ impl DataPayload<'static, CowStrMarker> { | |||
| /// `ErasedDataStruct` is to be used. | |||
| pub struct CowStringMarker; | |||
There was a problem hiding this comment.
Request: Please get rid of CowStringMarker now. I think it's only used in a couple of places.
There was a problem hiding this comment.
We use it in a bunch of tests; is there something it should be replaced with?
|
|
||
| /// All that Yoke needs from the Cart type is the destructor. We can | ||
| /// use a trait object to handle that. | ||
| pub(crate) type ErasedCart<'data> = Rc<dyn ErasedDestructor<'data> + 'data>; |
There was a problem hiding this comment.
Suggestion: I think ErasedCart is better suited for the yoke crate, both because it is more strongly coupled to Yoke than to DataPayload and because then we can keep the unsafe code in one crate.
There was a problem hiding this comment.
IMO it would be if it:
- did not have the lifetime
- did not have the
IsCovariant<'data>bound
These are both due to tricky constraints from our side and they make it not as general. My original plan was for it to be in the yoke crate but as long as we have the IsCovariant<'data> bound it doesn't make that much sense.
Once we get rid of the lifetime then this will be possible, I don't want to do it right now because the current design is too specific to us. Even if we can get rid of the IsCovariant bound I'd be okay with moving this upstream because ErasedCart<'static> would always work for folks who don't want to care about the lifetime. Requiring IsCovariant makes this much less generally useful
There was a problem hiding this comment.
There are also multiple ownership patterns one may want here; from an Rc to a Box.
There was a problem hiding this comment.
Will file a followup for moving this out
Manishearth
force-pushed
the
the-cart-the
branch
from
a61281e to
305cbf0
Compare
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Progress towards #1262
Builds on #1278
Fixes #1151
This does not completely get rid of RcStruct, however it gets rid of the need for the "cart" type and removes a lot of lifetimes. The biggest win we get here is that projections can be made freely between data payload types now.
It should be much easier to get rid of RcStruct after this.
Notes for reviewing: