search

helgoboss / reaper-rs

Rust bindings for the REAPER C++ API
MIT License
82 stars 8 forks source link

Consider `Option<u32>` for `size` in C-buffers. #72

Closed Levitanus closed 2 years ago

Levitanus commented 2 years ago

There are a lot of situations, when 99% of time you don't care of the size of string buffers.

When retrieving ExtState, or getting project info, which can be of any size.

I think, it's a good point to make the size parameter optional, for not having headache every time, you use a function in particular case.

For changing the policy, without affecting users code-base, it is possible to go in two ways, but quite similar in approach. Change the u32 type to impl Into<Option<u32>> and then match option with constant. For example, I don't know what the constant is. I tried u32::MAX, but it crashed my Reaper.

The decision, as I see, lays in the place this match should lay. In the with_*_buffer itself, or in every function, that can be annoying in usage. For example: it is better to get annoyed, when retrieving MIDI source, than be too expensive. Basically, I can implement either of them by myself, but I need the decision and direction :)

P.S. There could be two constants: reasonable size and max size.

helgoboss commented 2 years ago

There are a lot of situations, when 99% of time you don't care of the size of string buffers.

When retrieving ExtState, or getting project info, which can be of any size.

I agree.

I think, it's a good point to make the size parameter optional, for not having headache every time, you use a function in particular case.

Don't agree. It's out of scope of the medium-level API to add new logic, It's merely an adapter layer.

For changing the policy, without affecting users code-base, it is possible to go in two ways, but quite similar in approach. Change the u32 type to impl Into<Option<u32>> and then match option with constant. For example, I don't know what the constant is. I tried u32::MAX, but it crashed my Reaper.

That's another problem, you won't find a good default. It depends on the use case. Passing u32::MAX won't work because reaper-rs needs to allocate that many bytes of memory then, it's just too much. Higher level APIs can decide better which size to use.

It's just a limitation of the original REAPER API that reaper-rs can't solve. However, a recent REAPER version added a new technique using reallocation. This solves the problem and I want to include it at some point.

The decision, as I see, lays in the place this match should lay. In the with_*_buffer itself, or in every function, that can be annoying in usage. For example: it is better to get annoyed, when retrieving MIDI source, than be too expensive. Basically, I can implement either of them by myself, but I need the decision and direction :)

P.S. There could be two constants: reasonable size and max size.

Remember, the medium level API is not a convenience wrapper around the original REAPER API, its merely an adapter layer (which attempts to expose a more Rust-idiomatic interface, but only as far as possible, never by making it more than an adapter). If you want convenience, you need to move to a higher level.

Levitanus commented 2 years ago

Yes, later today a also thought about this...