std::c_vec is gone... what to do?

[drbawb]

I'm working on patching up after the recent changes from RFC 494.

The String changes are easy enough to fix: but CVec has been removed in its entirety.
The problem with CVec is that we use it quite extensively in the public API.

Should we just write a replacement for CVec that lives in rust-sdl2? (Maybe call it SDLVec or something?) The original implementation from the Rust stdlib is <200 lines of code including tests.

---

We could even take this as an opportunity to eliminate some duplicate code and allocations.
Most of our CVecs with destructors have the exact same destructor. (Call libc::free on the memory in question.) -- Yet every CVec w/ a dtor allocates an FnOnce to accomplish this.

For our use-case does the dtor really need to invoke a closure? Either we call libc::free or we don't -- so we could simplify it to a boolean flag and avoid all the closure machinery there.

[AngryLawyer]

Yikes. I'll need some time to ponder this.

[drbawb]

Another thought I had was trying to convert the data into standard Rust types:

• For the data we free: we could convert them to Vec<T> with from_raw_buf()
• For the data we don't free we could wrap it in std::raw::Slice and expose it as &'a [u8]

Though I don't necessarily like this approach because in the former case it implies another allocation and a copy into the Vec -- but I think it also fits in with the Rust ecosystem a little more cleanly.

---

FWIW the breakage is actually not too bad. After all the CString fix-age there are "only" 15 errors left and they're almost entirely in the render and audio systems. (Which is why I want to avoid allocations we don't need; wherever I call those systems it's always in very tight, performance-sensitive code.)

The real shame is just that we were exposing CVec in the public API so no matter what we do it's probably going to break quite a bit of downstream code.

[nukep]

I'll take a look at the audio module to see what can be done. Here's what I have in mind:

• AudioCVT is especially bizarre in the way it uses CVec and pointers IMO. I think CVec can be completely removed from it without unnecessary copies, and slices can be used in its place. ll::SDL_ConvertAudio simply mutates a preallocated buffer, so this should be trivial.
• AudioSpecWAV could own the buffer pointer (why doesn't it already... 😕 ), and be freed with an impl Drop for AudioSpecWAV.

I plan to send a pull request with the audio changes very soon. Feel free to cherry pick the changes if you wish.

[drbawb]

Here's what I've got so far, it's a working build: drbawb/rust-sdl2@b39bb23
EDIT: comparison to rust-sdl2/master

At the moment I just copied the last known revision of libstd/c_vec.rs out of the Rust tree just so I could get my build to the point where it would run type-checking and borrow-checking.

This passes checks on my machine and runs the two examples w/ no issues that I can see.

Any thoughts?

---

@nukep: I purposefully kept the commits which deal w/ CVec separate so it can be dropped during a rebase once you get audio.rs cleaned up.

[nukep]

Looks good to me. I would rather CVec be gone completely (it's only used in audio and render), but it's a good workaround for getting everything to compile right now.

I can reasonably determine that CVec can be removed from render as well; it's just a matter of using Vec and array slices in lieu of CVec. That can be done later, though. ;)

Just about done on my end. I'm running into issues with AudioCVT, so I'm removing it for now. Sending pull request soon.

[drbawb]

The only concern I have w/ constructing Vec<u8>s is that a CVec cannot grow/shrink, but a regular Vec can. When a Vec resizes it will move in memory: is that going to pose any problem on the SDL side of things?

Using your commit as a template I'm going to try to remove CVec from the render system before submitting a PR.

Shouldn't take me too long.

[nukep]

I don't think it'll cause any problems. The way they use CVec, it's to either allocate memory on the Rust side, or to reference an array owned by SDL (no freeing on the Rust end).

Here's my interpretation:

• read_pixels: the pixels variable is needlessly allocated with malloc, and could easily just become a Vec for the sake of storage. (EDIT: Growing/shrinking will not interfere with SDL because it's not owned or later used by SDL - it's allocated to be written to.)
• unsafe_lock: This is a private method used only by with_lock. I suggest combining the two, using the pixels and size variables to form a slice (see std::slice::from_raw_mut_buf()). with_lock<F: FnOnce(CVec<u8>, i32) -> ()> could become something like with_lock<F: FnOnce(&mut [u8], i32) -> ()>, where F accepts a slice instead.

[drbawb]

Alright I submitted a PR. I ended up having to use mem::transmute instead of the from_raw_buf() because fighting with the borrow checker lead to some really gnarly code.

(Plus the borrow-checker isn't helping us here anyways: the data is owned by SDL. It's not worth sacrificing the readability when we're already in an unsafe block; in my opinion anyways.)

---

👍 Looks good though! Thanks for the help.