Check for null pointer after MIDIPacketListAdd/MIDIEventListAdd#58
Check for null pointer after MIDIPacketListAdd/MIDIEventListAdd#58Boddlnagg wants to merge 4 commits intochris-zen:masterfrom
Conversation
MIDIPacketListAdd/MIDIEventListAdd can return null pointers. This leads to unsoundness. Ideally the error would be handled by the caller, but that requires a changed signature. So a panic is the best we can do for now.
|
The CI seems broken/outdated: |
Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>
Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>
Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>
|
Hi @Boddlnagg @xtqqczze thanks for the contributions. I was considering adding an alternative API that handles errors more explicitly (e.g., try_push / try_new), but I wanted to check whether it would actually be used in midir before investing time in it. Another thing I would consider is to release a major version that breaks compatibility but makes the push fallible. I would take the chance to create proper Errors instead of OSStatus. |
|
I would appreciate an additional fallible API. Using that in midir requires a breaking change there, but having it in this crate is a necessary precondition. |
|
@Boddlnagg One important question for me is whether to go for a major release with breaking changes (my preference), or to do a minor release with an additional fallible API (which would result in a less clean API). I’d really appreciate feedback from a user’s perspective, especially since midir is a key downstream user. |
|
I addressed this in #63 |
Why not both? It is an unsoundness after all, so you could do a minor release without breaking changes, with panic upon failure (this PR), and major release with breaking changes and better error handling. That way you leave it to the crate user to decide. |
|
Yeah, that makes sense. If you don't mind, I'll address this in my PR. I got different changes there to modernise the project. What version do you suggest? 0.8.2 or 0.9.0? |
|
I think by convention for pre-1.0 versions, 0.8.2 would typically be considered a minor release, whereas 0.9.0 would be treated more like a major bump. Of course, it’s still pre-1.0... |
MIDIPacketListAdd/MIDIEventListAdd can return null pointers. This leads to unsoundness (see #57).
Ideally the error would be handled by the caller, but that requires a changed signature. So a panic is the best we can do for now.