-
Notifications
You must be signed in to change notification settings - Fork 224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace lower_into_buffer and try_lift_from_buffer with a trait #972
Conversation
559e458
to
3518c67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! It's a nice cleanup, and also reduces potential for confusion 👍🏻.
uniffi/src/lib.rs
Outdated
} | ||
|
||
fn try_read(buf: &mut &[u8]) -> Result<Self> { | ||
T::try_read(buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistically, you're using all three of T::method
, ViaFfi::method
and <T as ViaFfi>::method
in calls here, it may be worth consolidating to a single one for consistency. (I personally like RustBufferViaFfi::method
as the most explicit option, but don't feel strongly about the details).
/// For complex types were it's too fiddly or too unsafe to convert them into a special-purpose | ||
/// C-compatible value, you can use this helper function to implement `lower()` in terms of `write()` | ||
/// and pass the value as a serialized buffer of bytes. | ||
pub fn lower_into_buffer<T: ViaFfi>(value: T) -> RustBuffer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess technically this is a breaking change, swapping some pub
functions for a pub
trait, and as such should be called out in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great - only adding this comment as I noticed a stray typo that you didn't even create, just moved :)
uniffi/src/lib.rs
Outdated
@@ -364,6 +338,44 @@ unsafe impl ViaFfi for String { | |||
} | |||
} | |||
|
|||
/// A helper trait to implement lowering/lifting using a `RustBuffer` | |||
/// | |||
/// For complex types were it's too fiddly or too unsafe to convert them into a special-purpose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo - "where" (that typo already existed :)
3518c67
to
b471be6
Compare
The main motivation is to eliminate the possibility of calling `try_lift_from_buffer()` on a `RustBuffer` that wasn't created with `lower_into_buffer()`. For example, this code seems reasonable, but it totally fails: ``` let buf: RustBuffer = ViaFfi::lower("Test".to_string()); assert_eq!(try_lift_from_buffer::<String>(buf).unwrap(), "Test") ``` This is because Strings have an asymmetry between `lower()` and `write()`. `write()` needs to write out the string length, but `lower()` can optimize to avoid that and just use `RustBuffer.len`. It also reduces some boilerplate code, which is nice too.
b471be6
to
f80642b
Compare
I'm not totally sure about this one, but it seems like it could be an improvement. What do you all think?
The main motivation is to eliminate the possibility of calling
try_lift_from_buffer()
on aRustBuffer
that wasn't created withlower_into_buffer()
. For example, this code seems reasonable, but ittotally fails:
This is because Strings have an asymmetry between
lower()
andwrite()
.write()
needs to write out the string length, butlower()
can optimize to avoid that and just useRustBuffer.len
.It also reduces some boilerplate code, which is nice too.