Questions before submitting a bunch of patches

[indygreg]

As I port python-zstandard to Rust, I'd like to use zstd-safe rather than zstd-sys. It is quite apparent that a lot of APIs are not yet available in zstd-safe. Before I submit a number of patches to implement missing functionality, I have a few questions:

1. Will you accept PRs adding safe wrappers for most/all zstd API primitives?
2. Is there a reason zstd_safe uses individual functions instead of grouping functionality for types via impl T { }. e.g. why define fn create_cctx() instead of impl CCtx { pub fn create() }?
3. Related to the prior question, do you mind if I use impl T { } to hold functionality for new types? (If not, I totally understand and don't mind following existing conventions.)
4. I noticed that the pointer returned by various ZSTD_create* APIs is never checked for NULL. This seems like a potential NULL access bug. Are you aware of this? This will likely necessitate using Result on functions that could fail. Are you OK with patches that change the non-robust functions to return Result<T> instead? (This will be a backwards incompatible break.) Alternatively, we could introduce new APIs that return Result<T> and preserve the existing APIs for backwards compatibility.

Thank you for maintaining this project!

[gyscos]

Hi, and thanks for the interest!

1. Will you accept PRs adding safe wrappers for most/all zstd API primitives?

Absolutely! I started with the minimal amount needed for zstd-rs, but the goal is indeed to cover the entire API.

2. Is there a reason zstd_safe uses individual functions instead of grouping functionality for types via impl T { }. e.g. why define fn create_cctx() instead of impl CCtx { pub fn create() }?

I've been thinking about grouping it like that. I tried at first to be as thin as possible, and to mimic the C API almost 1-for-1, but it probably makes sense to at least use inherent implementation.

3. Related to the prior question, do you mind if I use impl T { } to hold functionality for new types? (If not, I totally understand and don't mind following existing conventions.)

Sure, let's try to get a nice API!

4. I noticed that the pointer returned by various ZSTD_create* APIs is never checked for NULL. This seems like a potential NULL access bug. Are you aware of this? This will likely necessitate using Result on functions that could fail. Are you OK with patches that change the non-robust functions to return Result<T> instead? (This will be a backwards incompatible break.) Alternatively, we could introduce new APIs that return Result<T> and preserve the existing APIs for backwards compatibility.

True, I didn't think much about that. I think I would panic on null returns in the default functions, and provide Result-returning ones for when the user wants to recover - much like the default allocation API from the rust std lib.

Thank you for maintaining this project!

Thank you for helping and contributing! :)

[gyscos]

Some time has passed, and the situation has a bit improved:

• zstd_safe now has a slightly more idiomatic API with methods on the various types rather than free functions
• we now use NonNull<T> rather than just *mut T, which automatically checks for null pointers. As such, all the create() method now have a try_create which returns None when zstd returns a null pointer.

We're still not covering 100% of the C library, so things can still improve.