Skip to content

Conversation

@RoflCopter24
Copy link
Contributor

Instead of taking the easy way out and killing the whole program by panicking, device enumeration and stream creation will now report the error variant 'Unknown' as suggested in #216

Also added an "InvalidArgument" variant for the infamous -22 error (this should resolve #247 and make #206 more handable)

Instead of taking the easy way out and killing the whole program by panicking, device enumeration and stream creation will now report the error variant 'Unknown'
@richard-uk1
Copy link
Contributor

LGTM I was playing with something like this myself in #223 (comment)

@richard-uk1
Copy link
Contributor

I'm just going to take this opportunity to moan about the awful error reporting in alsa, and C libs in general. They give you nothing to work with but a meaningless posix error code.

@RoflCopter24
Copy link
Contributor Author

RoflCopter24 commented Nov 7, 2018

@derekdreery I really like your idea to get the error message directly from alsa. That should enable more differentiated error handling so that if you get some weird error, at least you get somewhat of an idea what went wrong.

That beeing written, let me add some rant as well:
I very much dislike the prominence with which Rust advertises panics - I think they should not be used in library crates at all. If a library fails its task, there is absolutely no reason to crash a whole thread or maybe even the whole program by panicking in some obscure location. That's what Results/Options are for!
Often I get the impression, that ppl are too lazy to implement proper error handling and think 'I throw this panic in here, so I can get on with my life and be done'.

@richard-uk1
Copy link
Contributor

@RoflCopter24 I agree with you that panicking is often the wrong thing to do for stable libraries, but all libraries started out as prototypes, and when you're prototyping you don't want to spend ages on errors. It just needs someone to improve error handling here, which anyone can do I think.

@desttinghim
Copy link
Contributor

What is holding this Pull Request back?

@richard-uk1
Copy link
Contributor

Personally, I think this should be merged, as it is an improvement on the current state of things.

@desttinghim
Copy link
Contributor

@tomaka Can you comment?

Copy link
Member

@mitchmindtree mitchmindtree left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

While I'm not sure how to feel about the current approach to cross-platform errors in CPAL generally, I agree this at least follows suit with the existing implementation and is nicer than panicking internally.

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.

cpal crash on alsa backend (beep example)

4 participants