Conversation
4f972f1 to
817eb9f
Compare
joncinque
left a comment
There was a problem hiding this comment.
Mostly small points, looks good to me!
As another naming idea, what we're really providing is a View on existing data, so it's like a Vec that won't reallocate, which also similar to an arena allocator.
So we could go with ListView or NoAllocVec or ArenaList. I kind of like the last one, since it might describe the situation best.
febo
left a comment
There was a problem hiding this comment.
Looks very nice – left a few comments. The main one is about the alignment. It would be nice if we could make it more flexible to support types 8-byte aligned.
I have a slight preference for |
Agreed, I personally have never heard the term "arena" in this context before, and I assume most developers will whiff on the underlying meaning at first. However, "view" implies readonly, and that's not the case here. Here are a few ideas:
|
Agree with your point on the "View" implying read-only, but while the type has a "dynamic" size its capacity is fixed. In that sense it is a view. 😊 The only concern that I have with using |
grod220
left a comment
There was a problem hiding this comment.
Sorry for the delay on this! Wasn't quite sure we were going to need this until recent confirmations in the Unstake program.
817eb9f to
56057f1
Compare
buffalojoec
left a comment
There was a problem hiding this comment.
Awesome!! This is looking fantastic, and the diagrams under unpack_internal are really an elegant touch for helping follow the slice processing.
grod220
left a comment
There was a problem hiding this comment.
A few things I am realizing.
1 - With unstake program’s new settlement design, we no longer need the remove functions.
2 - Users of ListView will not be able to use PodSlice for read-only use (if they rely upon the default len bytes). Pod64 vs Pod32 defaults. Given ListView now has iterable methods, it sort of seems like it could replace both PodSlice & PodSliceMut.
Up for discussion if this is worth keeping or not. The benefits are better alignment handling, remove functions, and test coverage. But perhaps these features are not pressing any longer.
Are we totally sure we don't need it yet, though? I know we may not need |
Yes, iterables on the mutable version are definitely useful. At the moment, it's awkward. We are reading as PodSlice, doing iteration, and then immediately later reading the bytes as PodSliceMut to make edits. With the fixes for this + alignment updates, I think there is an argument to ship this regardless. However, the more full-featured ListView becomes, it sort of doesn't make sense for PodSlice to be used at all and that should be likewise deprecated. |
Yep, I would agree! |
joncinque
left a comment
There was a problem hiding this comment.
Looks great overall! Mostly small things, that you can take or leave in future work
bda8ca8 to
4fd0c2e
Compare
|
Having the mutable New API pattern
Both of them implement let size = ListView::<u32>::size_of(3).unwrap();
let mut buffer = vec![0u8; size];
let mut list = ListView::init(&mut buffer).unwrap();
list.push(42u32).unwrap();
list.push(100u32).unwrap();
// Later, unpack the same buffer as read-only
let read_list = ListView::<u32>::unpack(&buffer).unwrap();
assert_eq!(read_list.len(), 2); |
4fd0c2e to
1e9df09
Compare
e09c5ea to
376e2bf
Compare
joncinque
left a comment
There was a problem hiding this comment.
Just some small things, looking great overall!
buffalojoec
left a comment
There was a problem hiding this comment.
Nice!! This is super useful.
Motivation
The existing PodSlice had several limitations:
Changes made
This PR introduces a new list module that provides a fully‑featured, zero‑copy, variable‑length collection API: ListView, ListViewMut, & ListViewReadOnly.
Has general‑purpose length prefixes – PodLength lets any of the pod‑ints act as the length type.
PodSliceandPodSliceMutare now marked as deprecated.