{"action":"created","issue":{"url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/270","labels_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/270/labels{/name}","comments_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/270/comments","events_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/270/events","html_url":"https://github.com/AngryLawyer/rust-sdl2/issues/270","id":53791888,"number":270,"title":"RFC: Restore AudioCVT Interface (w/o CVec)","user":{"login":"drbawb","id":235438,"avatar_url":"https://avatars.githubusercontent.com/u/235438?v=3","gravatar_id":"","url":"https://api.github.com/users/drbawb","html_url":"https://github.com/drbawb","followers_url":"https://api.github.com/users/drbawb/followers","following_url":"https://api.github.com/users/drbawb/following{/other_user}","gists_url":"https://api.github.com/users/drbawb/gists{/gist_id}","starred_url":"https://api.github.com/users/drbawb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/drbawb/subscriptions","organizations_url":"https://api.github.com/users/drbawb/orgs","repos_url":"https://api.github.com/users/drbawb/repos","events_url":"https://api.github.com/users/drbawb/events{/privacy}","received_events_url":"https://api.github.com/users/drbawb/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":8,"created_at":"2015-01-08T19:43:21Z","updated_at":"2015-01-10T00:51:08Z","closed_at":null,"body":"So in PR #269 AudioCVT was removed in a commit by @nukep \r\n\r\nI have a patch that replaces this w/ an implementation that uses a caller allocated Vec<u8>, let me know what you think: drbawb/rust-sdl2@aa42ceeba3ca3af386a7b8e7a9d2c2c0c146e9bf\r\n\r\nA couple key areas to review: \r\n* `len`, `len_mult`, and now `len_cvt` are int32 in SDL2; but interpreted as uint's in the bridge:\r\n  * Are they guaranteed to never be negative? \r\n  * Should we add a `debug_assert!` to that effect?\r\n\r\n* I set the length of the resultant `Vec<u8>` _before checking the error code:_\r\n  * Is `len_cvt` well defined if the conversion failed?\r\n  * Should we defer setting the length until after checking the return code?\r\n\r\n* Style nit: `std::mem, std::raw` are presently imported by the functions which use them; rather than at the module level. \r\n  * I actually like this; it feels like it contains the \"unsafe imports\" to unsafe areas of the code.\r\n  * However it does mean we have duplicate imports in several functions now...\r\n\r\n---\r\n\r\nLet me know if this looks worthy of a PR."},"comment":{"url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/comments/69427412","html_url":"https://github.com/AngryLawyer/rust-sdl2/issues/270#issuecomment-69427412","issue_url":"https://api.github.com/repos/AngryLawyer/rust-sdl2/issues/270","id":69427412,"user":{"login":"nukep","id":1551458,"avatar_url":"https://avatars.githubusercontent.com/u/1551458?v=3","gravatar_id":"","url":"https://api.github.com/users/nukep","html_url":"https://github.com/nukep","followers_url":"https://api.github.com/users/nukep/followers","following_url":"https://api.github.com/users/nukep/following{/other_user}","gists_url":"https://api.github.com/users/nukep/gists{/gist_id}","starred_url":"https://api.github.com/users/nukep/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nukep/subscriptions","organizations_url":"https://api.github.com/users/nukep/orgs","repos_url":"https://api.github.com/users/nukep/repos","events_url":"https://api.github.com/users/nukep/events{/privacy}","received_events_url":"https://api.github.com/users/nukep/received_events","type":"User","site_admin":false},"created_at":"2015-01-10T00:51:08Z","updated_at":"2015-01-10T00:51:08Z","body":"It should at least be marked as `#[unstable]` IMO, but I don't know about marking it as `unsafe`. `AudioCVT#convert()` is only \"unsafe\" because of an underlying bug in SDL; the nature of the operation is not fundamentally unsafe, i.e. shouldn't require an `unsafe { }` block. Additionally, whether or not `AudioCVT#convert()` is bug-prone completely depends on which version of SDL is linked.\r\n\r\nI absolutely agree, though; this issue should be documented to the caller."}}