Skip to content

Conversation

HEnquist
Copy link
Contributor

@HEnquist HEnquist commented Aug 7, 2025

This library uses quite a few constants, for audio unit types, errors, etc etc. Currently, they are defined directly in the core audio-rs code. But their original definitions are in the sdk headers, and that means they are available in the objc2 crates.

This PR removes all the constant defined in this library, and instead imports them from objc2. This way there Is much less risk for typos, and if there is a typo is much easier to spot. The only exception is the kAudioUnitSubType_RemoteIO subtype which only exists in the iOS sdk. I have not managed to figure out how to import it, so I left this one as it was.

The api almost doesn't change. Only one missing BadData error is added, and some incomplete mapping between os status and error is fixed.

Copy link
Member

@simlay simlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a bit of time trying to find a case of inconsistency in the actual integer values. I found that using cargo-semver-checks to actually help me see which of the enum fields actually changed their usize key - the enum usize keys that changed were the ones that never existed. Given that we've got a breaking change in the works with #137 (a now unsafe function), it's a good time to clean up and break a few things.

Anyway, this is nice! Well done!

Comment on lines +37 to +43
_ if os_status == kAudio_UnimplementedError => Err(Error::Unimplemented),
_ if os_status == kAudio_FileNotFoundError => Err(Error::FileNotFound),
_ if os_status == kAudio_FilePermissionError => Err(Error::FilePermission),
_ if os_status == kAudio_TooManyFilesOpenError => Err(Error::TooManyFilesOpen),
_ if os_status == kAudio_BadFilePathError => Err(Error::BadFilePath),
_ if os_status == kAudio_ParamError => Err(Error::Param),
_ if os_status == kAudio_MemFullError => Err(Error::MemFull),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this gross but don't have a better suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's really ugly. I have the same here: https://github.com/HEnquist/wasapi-rs/blob/091e0da655a9a33eaeab035f3e373fdd34d5f6f3/src/api.rs#L464
I'll play around with making a macro for this, should be fairly straightforward to make something that isn't so painful to look at.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a macro, this could be written in this way instead:

            gen_match! {
                os_status => {
                    noerr => Ok(()),
                    kAudio_UnimplementedError => Err(Error::Unimplemented),
                    kAudio_FileNotFoundError => Err(Error::FileNotFound),
                    kAudio_FilePermissionError => Err(Error::FilePermission),
                    kAudio_TooManyFilesOpenError => Err(Error::TooManyFilesOpen),
                    kAudio_BadFilePathError => Err(Error::BadFilePath),
                    kAudio_ParamError => Err(Error::Param),
                    kAudio_MemFullError => Err(Error::MemFull),
                    _ => Err(Error::Unknown),
                }
            }

So now we have a nicer looking match. But we also have an ugly and hard to read macro:

    macro_rules! gen_match {
    ($expr:expr => { $($const:ident => $out:expr),* , _ => $default:expr $(,)? }) => {
        match $expr {
            $(
                _ if $expr == $const => $out,
            )*
            _ => $default,
        }
    };

Did we gain anything? I'm not convinced..

@simlay simlay merged commit 2d23bac into RustAudio:master Aug 9, 2025
7 checks passed
@HEnquist
Copy link
Contributor Author

HEnquist commented Aug 9, 2025

That semver checking tool is really useful. I didn't know about it but I will start using it. Thanks for the tip :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants