Implement iter_arc method for List#71
Conversation
|
@michaelsproul is this a good start? do let me know |
|
Yeah this looks good. I wonder if we could write the ordinary |
|
It would be great to add a new milhouse/src/tests/proptest/operations.rs Lines 210 to 212 in e1c9a57 However it would have to run conditionally on |
PoulavBhowmick03
force-pushed
the
list/iter_arc
branch
from
0bfd0f0 to
ec8d212
Compare
|
Needs some merge conflicts resolved, I've just been preparing a v0.7.0 release |
No problem. Fixing it |
PoulavBhowmick03
force-pushed
the
list/iter_arc
branch
from
ec8d212 to
ae3e13a
Compare
|
@michaelsproul fixed the conflict and the clippy warning |
|
Thanks, will review thoroughly and probably merge tomorrow. Looked good at a glance |
| } | ||
|
|
||
| pub fn iter_arc(&self) -> Result<ArcIter<'_, T>, Error> { | ||
| self.backing.iter_arc(0) |
There was a problem hiding this comment.
I think this might be wrong in the case where there are pending updates. The proptest should have caught this
There was a problem hiding this comment.
I was thinking to change it to use ArcInterfaceIter, which handles the pending updates. But in that case, I will have to change the ImmList<> and include <U: UpdateMap<T>> to it. what would you suggest is the way to go about this?
| } | ||
| Op::IterArc => { | ||
| if <T as TreeHash>::tree_hash_type() != TreeHashType::Basic { | ||
| list.apply_updates().unwrap(); |
There was a problem hiding this comment.
This should not be necessary, we should make iter_arc work without it
There was a problem hiding this comment.
The reason being: requiring the caller to keep track and flush pending updates in order for a method to work makes it a footgun. Take a look at InterfaceIter to see how it handles the pending updates. We can do the same for ArcIter.
Eventually I'd like to rethink how pending updates are handled in Milhouse, but I've gotten a bit stuck on previous attempts, see:
There was a problem hiding this comment.
That makes sense, will take a look into this PR and how pending updates are handled in InterfaceIter as well
There was a problem hiding this comment.
Mmm, this is hard. I almost want to try implementing UpdateMap for BTreeMap<usize, Arc<T>> so that we aren't doing useless Arcing
|
Thanks for your patience @PoulavBhowmick03 🙏 Left a few more review comments. Sorry they're a bit nitpicky but I think we're getting close! |
|
@michaelsproul I have addressed all the issues except for one, left a reply to that, above |
PoulavBhowmick03
force-pushed
the
list/iter_arc
branch
from
ace8fed to
90452a1
Compare
| } | ||
|
|
||
| fn get_arc(&self, k: usize) -> Option<Arc<T>> { | ||
| self.get(&k).cloned().map(Arc::new) |
There was a problem hiding this comment.
This is unfortunate because it's just creating a new Arc.
Maybe we could have this as a fallback impl, and for maps that support it (like BTreeMap<usize, Arc<T>> we can have an efficient impl). The problem will be overlapping impls. We could get around that with a new type like struct ArcMap<T>(VecMap<usize, Arc<T>>).
This is all turning out to be a bit complicated haha
PoulavBhowmick03
force-pushed
the
list/iter_arc
branch
from
90452a1 to
a8ae440
Compare
2 participants
Fixes #70
Introduced method
iter_arc:iter_arc.rsiter_arcmethod iterates over and gets each element of the List wrapped inArc<T>.Listimpl inlist.rs