Aggregate FFI client code in one common place. #133
Conversation
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
| #[allow(rustdoc::private_intra_doc_links)] | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn create_client_using_config( | ||
| config: *const ConnectionConfig, |
There was a problem hiding this comment.
I recommend that you don't rely on structs in public APIs, since it requires a caller that can setup a struct - this capability isn't universally available. I know it's verbose, but it's much better to just take all of the arguments instead of wrapping them in a struct.
It also removes the need to manage the struct's lifetime around this call.
There was a problem hiding this comment.
This is somewhat problematic, because sometimes we need to return multiple values to the wrapper (errors, complex values). Is there an alternative that doesn't involve protobuf?
There was a problem hiding this comment.
not to do this in a unified way, instead implementing this for every language according to its capabilities. 🤷
I agree its problematic, but it's better to face this problem ahead of time instead of creating an API and later understanding that it won't work in some situations.
There was a problem hiding this comment.
Okay, I think for now, we'll just have a separate lib.rs for each wrapper instead of taking this unified approach. If we find there are some parts that can reasonably be shared later on, we can move those to a shared library later.
| /// * `connection_request_bytes` must point to `connection_request_len` consecutive properly initialized bytes. | ||
| /// * `connection_request_len` must not be greater than `isize::MAX`. See the safety documentation of [`std::slice::from_raw_parts`](https://doc.rust-lang.org/std/slice/fn.from_raw_parts.html). | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn create_client_using_protobuf( |
There was a problem hiding this comment.
this requires that the caller will have a dependency on protobuf, which we want to avoid.
glide-core/src/ffi/ffi_client.rs
Outdated
| RequestType::PTTL => Some(cmd("PTTL")), | ||
| RequestType::ZRemRangeByRank => Some(cmd("ZREMRANGEBYRANK")), | ||
| RequestType::Persist => Some(cmd("PERSIST")), | ||
| _ => None, |
There was a problem hiding this comment.
If this case happens, this is considered UB, even if we try to switch this to a non-exhaustive enum. See here: https://www.reddit.com/r/rust/comments/cfp6lt/non_exhaustive_and_ffi/
We should probably just mark this as a safety condition in the documentation.
There was a problem hiding this comment.
or follow the good advice from the link you provided, and implement TryFrom for the enum.
Aggregating together things (rust) from