Test VM should use Rc<Blockstore> for consistency with the MockRuntime #1454
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1454 +/- ##
==========================================
- Coverage 91.05% 91.00% -0.06%
==========================================
Files 147 147
Lines 27982 27986 +4
==========================================
- Hits 25479 25468 -11
- Misses 2503 2518 +15
|
test_vm/src/lib.rs
Outdated
| @@ -82,15 +83,15 @@ impl<'bs> TestVM<'bs> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn new_with_singletons(store: &'bs MemoryBlockstore) -> TestVM<'bs> { | |||
| pub fn new_with_singletons(store: Rc<MemoryBlockstore>) -> TestVM { | |||
There was a problem hiding this comment.
I'd consider defining this as store: impl Into<Rc<MemoryBlockstoire>>, allowing the user to pass either a MemoryBlockstore or an Rc<MemoryBlockstore>>. I.e.
| pub fn new_with_singletons(store: Rc<MemoryBlockstore>) -> TestVM { | |
| pub fn new_with_singletons(store: impl Into<Rc<MemoryBlockstore>>) -> TestVM { | |
| let store = store.into(); |
Same with TestVM::new.
But let's wait for @anorth to comment on this before continuing.
There was a problem hiding this comment.
This would let us call TestVM::new_with_singletons(store) without having to explicitly wrap in an Rc.
There was a problem hiding this comment.
Done for both. Great idea !
There was a problem hiding this comment.
@Stebalien @anorth Where is the implementation for
impl Into<Rc<MemoryBlockstore>> for MemoryBlockstore {
}
defined though in the code ? I also don't see a
impl From<MemoryBlockstore> for Rc<MemoryBlockstore>
anywhere. Does the Rust stdlib have come default impl here to convert a MemoryBlockstore to an Rc ?
There was a problem hiding this comment.
First, there's also a blanket implementation of Into<U> for T where U: From<T>. That is, you always implement From<T> for U and get an automatic implementation of Into<U> for T. Never implement Into directly. I recommend reading the Into documentation and the From and Into section of the book.
Additionally, there's a blanket implementation of From<T> for Rc<T>.
There was a problem hiding this comment.
Thanks @Stebalien ! Yeah the blanket implementation of From<T> for Rc<T> is what I missing on.
test_vm/src/lib.rs
Outdated
| @@ -82,15 +83,15 @@ impl<'bs> TestVM<'bs> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn new_with_singletons(store: &'bs MemoryBlockstore) -> TestVM<'bs> { | |||
| pub fn new_with_singletons(store: Rc<MemoryBlockstore>) -> TestVM { | |||
|
No issues from me |
test_vm/src/lib.rs
Outdated
| TestVM { | ||
| primitives: FakePrimitives {}, | ||
| store, | ||
| store: store, |
There was a problem hiding this comment.
| store: store, | |
| store, |
There was a problem hiding this comment.
This is what clippy is complaining about.
| let store = Rc::new(MemoryBlockstore::new()); | ||
| let v = TestVM::new_with_singletons(store); |
There was a problem hiding this comment.
| let store = Rc::new(MemoryBlockstore::new()); | |
| let v = TestVM::new_with_singletons(store); | |
| let store = MemoryBlockstore::new(); | |
| let v = TestVM::new_with_singletons(store); |
Given the changes to the new_with_singletons signature, we shouldn't need to manually wrap in an Rc. Apply below as well.
There was a problem hiding this comment.
Fixed here and elsewhere.
3b870f9 to
3b3b87f
Compare
|
@Stebalien Addressed your review and put this in the merge queue. |
Follow up to #1446 for #1446 (comment).