-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better unified piece abstractions using a tiny bit of unsafe
#1287
Conversation
52dd45a
to
3903c5a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sooo much better. It's great to see core primitives being synergistic like Lego bricks.
/// This version is allocated on the heap, for stack-allocated piece see [`PieceArray`]. | ||
/// | ||
/// Internally piece contains a record and corresponding witness that together with records root of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is weird, though. Maybe we can make:
Piece
for regular piece on stackVecPiece
for heap allocated one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to keep it for now and see how codebase evolves. Piece
was the canonical data structure before and it actually looked a lot like current PieceArray
, but then it got bigger and it was clear that with future expansion placing it on the stack is not an option.
Now that we have PieceArray
and much more usable/useful FlatPieces
we can probably reduce reliance on Piece
and even replace it with direct Box<PieceArray>
where it still remains and get rid of current Piece
. But I'd prefer to keep things as is for now and see how it plays out.
… remove the need of `mem::transmute` in some cases
|
||
impl From<Piece> for Vec<u8> { | ||
fn from(piece: Piece) -> Self { | ||
piece.0 | ||
piece.0.to_vec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't do an extra allocation:
https://doc.rust-lang.org/std/primitive.slice.html#method.into_vec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an array, not a slice. Method doesn't seem to exist here. I could likely work around with unsafe
, but the whole point of this exercise was to avoid it.
Previous iteration relied on safe code only and had usability shortcomings. With just a bit of
unsafe
in strategic places it was possible to make API A LOT better. I'm actually proud of what I have achieved here in the end.The main changes are in
pieces.rs
. Many of the methods were wrappers round memory slices, but withtransmute
we can turn references to arrays of bytes into references to transparent data structures containing arrays of bytes. With reference out of the data structure we don't need a pair of data structures for mutable and immutable reference scenarios.The biggest piece of code is related to serde support. I basically took previous expanded version and adjusted it to new internal layout of
FlatPieces
because derivation was no longer possible.Code contributor checklist: