-
Notifications
You must be signed in to change notification settings - Fork 25
Atom soundness and usability fixes #100
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
Merged
Merged
Conversation
This file contains hidden or 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
3 tasks
… to check them using an enum
…cator to a more accurate SpaceWriter
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
SpaceAPIs), 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.SpaceintoAlignedSpace, and made it a Dynamically Sized Type. This fixes a good amount of lifetime issues, allowAlignedSpaces to also be writable. This type's only role is to ensure its contents are aligned for reading/writing a given typeT.use_paddingparameters in atom writing methods, they are dropped in favor of internally usingAlignedSpaces which align themselves automatically depending on the input type.SpaceReaderstruct, a cursor that contains many ergonomic methods for reading data from Atom buffers, to be used by custom Atomreadimplementations, also making them much safer.split_*methods onSpace. TheSpaceReaderis to be used instead.VecSpace, aVec-backed Space buffer, that can grow when writing Atoms into it. It replaces the quirkySpaceListthat used a linked list implementation.FramedMutSpaceintoAtomSpaceWriter. Its implementation also has been refactored to use indexes instead of pointers,to be compatible with re-allocating writers such as
VecSpace.RootMutSpaceintoSpaceCursor.MutSpacetrait 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,SpaceCusrorandAtomSpaceWriteractually implement, andthe
SpaceWriteris automatically implemented for those via a blanket implementation.Terminatedstruct, a wrapper around anySpaceAllocatorthat 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
StringofMidiEventatom writers, where a terminator byte has to be inserted but multiple writes can be made.This also makes them not reliant on their
Dropdestructors running to write the final byte, removing a potential source of UB.UnidentifiedAtoma 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.
AtomHeaderstruct, a thin wrapper over theLV2_Atomheader 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.ReadParameterandWriteParameterassociated 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-atomcrate is exposing.Atomtype. 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).
Vectortype 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.Sequenceunit type reading into a type parameter for its reader. This removes many subsequent checks, aswell as the need for the
TimeStampURIDenum, as a sequence's timestamp unit type is known ahead of time now.AtomErrorerror type, as well as sub-error typesAtomReadError,AtomWriteError, andAlignmentError.Now all atom-manipulating methods return these errors in a
Resultinstead of anOption, helping with semantics andmaking debugging a bit easier.
preludefor 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!