Implement ObjectStore for Arc<T> and Box<T> #526
Conversation
|
I think the reasoning for The reasoning for |
I did Box because it is pretty common for traits to implement |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks for this contribution @wyatt-herkamp
src/lib.rs
Outdated
| as_ref_impl!(Arc<dyn ObjectStore>); | ||
| as_ref_impl!(Box<dyn ObjectStore>); | ||
|
|
||
| macro_rules! as_ref_generic_impl { |
There was a problem hiding this comment.
this macro looks very similar to as_ref_impl -- if we implement for all Arc<T>/Box<T> could we remove the explicit implementations for Arc<dyn ..> and Box<dyn ..> 🤔
There was a problem hiding this comment.
We could, but I think it would be a breaking change. I can merge the macros together by adding a second brand on the as_ref_impl macro if you would like?
There was a problem hiding this comment.
It would be a breaking change?
There was a problem hiding this comment.
Well, if we remove the implementation for Arc<dyn ...> and Box<dyn ...>, anyone who uses those implementations might have a problem.
Merging the macros isn't breaking. But removing those existing implementations might cause issues
There was a problem hiding this comment.
My understanding of
if we implement for all
Arc<T>/Box<T>
was that dyn ObjectStore already implemented T, so if you implement impl<T: ObjectStore> ObjectStore for Arc<T>, then maybe that already covers Arc<dyn ObjectStore>?
There was a problem hiding this comment.
Oh I just remembered something!
Rust automatically adds the Sized bound. We need to add + ?Sized
There was a problem hiding this comment.
Ok T: ObjectStore doesn't cover Box<dyn ObjectStore> but T: ObjectStore + ?Sized does.
The same applies to Arc<T>
wyatt-herkamp
force-pushed
the
wyatt/as_ref_generic_impl
branch
from
bd2a791 to
da1add7
Compare
|
Rebased after the addition of ObjectStoreExt trait |
3 participants
Which issue does this PR close?
Closes #525
Rationale for this change
Allows you to put
Arc<T>into something like structObjectStoreWrapper<T: ObjectStore>(T)What changes are included in this PR?
Implementing ObjectStore for
Arc<T>andBox<T>Are there any user-facing changes?
No and this should not break anything. Just expand the flexibility of the ObjectStore Trait.