-
Notifications
You must be signed in to change notification settings - Fork 23
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
Atom soundness and usability fixes #100
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
prokopyl
added
📖 Documentation
An effort to improve the documentation
💥 Unsound API
A bug that allows to trigger Undefined Behavior in safe Rust
💔 Breaking change
Changes to the code that will lead to break one or more APIs
🌟 Ergonomics
Little things that matter! Does not add functionality, but makes an API easier to use
labels
Sep 26, 2021
3 tasks
… to check them using an enum
…cator to a more accurate SpaceWriter
prokopyl
force-pushed
the
atom-soundness
branch
from
November 28, 2021 20:41
d2f3354
to
8eb5f65
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
📖 Documentation
An effort to improve the documentation
💥 Unsound API
A bug that allows to trigger Undefined Behavior in safe Rust
💔 Breaking change
Changes to the code that will lead to break one or more APIs
🌟 Ergonomics
Little things that matter! Does not add functionality, but makes an API easier to use
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on the options feature (#98), I realized there were many issues in the Atom APIs
(mainly in the
Space
APIs), and they needed quite a bit of work. The main issues were lifetime issues, and quitea few unsoundness issues (due to methods not being marked unsafe, and alignment reading issues).
Turns out it was quite a lot in the end, but I think this makes a lot of the APIs clearer and easier to use as well. :)
unsafe
, as they have to blindly trust the contents of the input buffer to be containing atoms.Space
intoAlignedSpace
, and made it a Dynamically Sized Type. This fixes a good amount of lifetime issues, allowAlignedSpace
s to also be writable. This type's only role is to ensure its contents are aligned for reading/writing a given typeT
.use_padding
parameters in atom writing methods, they are dropped in favor of internally usingAlignedSpace
s which align themselves automatically depending on the input type.SpaceReader
struct, a cursor that contains many ergonomic methods for reading data from Atom buffers, to be used by custom Atomread
implementations, also making them much safer.split_*
methods onSpace
. TheSpaceReader
is to be used instead.VecSpace
, aVec
-backed Space buffer, that can grow when writing Atoms into it. It replaces the quirkySpaceList
that used a linked list implementation.FramedMutSpace
intoAtomSpaceWriter
. Its implementation also has been refactored to use indexes instead of pointers,to be compatible with re-allocating writers such as
VecSpace
.RootMutSpace
intoSpaceCursor
.MutSpace
trait intoSpaceWriter
. All the helper methods are now implemented directly ontothis trait instead of on the obnoxious
dyn MutSpace
. An object-safe sub-traitSpaceWriterImpl
(to be better namedat some point) is what the allocators such as
VecSpace
,SpaceCusror
andAtomSpaceWriter
actually implement, andthe
SpaceWriter
is automatically implemented for those via a blanket implementation.Terminated
struct, a wrapper around anySpaceAllocator
that adds a terminator byte at the very end of anyallocation, overwriting any previous terminator bytes it may have written. This is very useful to implement things like
the
String
ofMidiEvent
atom writers, where a terminator byte has to be inserted but multiple writes can be made.This also makes them not reliant on their
Drop
destructors running to write the final byte, removing a potential source of UB.UnidentifiedAtom
a Dynamically Sized Type, already containing a valid header. This allows to remove manyunneeded extra checks, as the type itself guarantees there's a valid header in there.
AtomHeader
struct, a thin wrapper over theLV2_Atom
header struct that allows to safely manipulateheaders as a whole. Unlike
LV2_Atom
, it also has a required 64-bit alignment, as per the LV2 spec.ReadParameter
andWriteParameter
associated types. This massively reduces clutter in methods that read atoms,and makes intent clearer as to what operation is being done. Atom types that needed those parameters now return an intermediary struct
using the typestate pattern.
lv2-atom
crate is exposing.Atom
type. They are replaced with a GAT-like workaround, where the ReadHandle andWriteHandle now return family structs
implementing a new family trait,
AtomHandle<'a>
. This approach is a bit more verbose when implementing atom types,but allows atom reading methods to be properly generic over lifetimes, both improving usability and removing UB (as
incorrect lifetime assumptions could be made).
Vector
type parameter. The type parameter and urid check has been moved into its reader type. This alsofixes the unneeded type parameters when trying to store a
Vector
's URID.Sequence
unit type reading into a type parameter for its reader. This removes many subsequent checks, aswell as the need for the
TimeStampURID
enum, as a sequence's timestamp unit type is known ahead of time now.AtomError
error type, as well as sub-error typesAtomReadError
,AtomWriteError
, andAlignmentError
.Now all atom-manipulating methods return these errors in a
Result
instead of anOption
, helping with semantics andmaking debugging a bit easier.
prelude
for the atom-implementing side. This is mainly to de-clutter the main prelude, since themajority of users will not be implementing their own Atom types.
AlignedSpace
.I may be adding some extra documentation, but otherwise I consider this PR to be pretty complete!