Add FFI for FixedDecimalFormat #680
Conversation
|
The lifetimes were not much of a problem, but we'll need to document them diligently. A nice thing is that the |
| @@ -0,0 +1,49 @@ | |||
| macro_rules! c_enum { | |||
There was a problem hiding this comment.
I added one of these for enums but not structs because structs have the additional complexity that each field can need Into, which we can solve by either:
- call
into()on everything (allows for changes in the field type to get missed) - allow an
issyntax in the field level, which does make the macro trickier to write (but still doable)
|
This is now ready for review! |
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
Codecov Report
@@ Coverage Diff @@
## main #680 +/- ##
==========================================
- Coverage 74.24% 73.75% -0.49%
==========================================
Files 171 175 +4
Lines 10824 10865 +41
==========================================
- Hits 8036 8014 -22
- Misses 2788 2851 +63
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build 1de26b34cda15b42b52d6143e01183ed0e35f7f8-PR-680
💛 - Coveralls |
| /// - `buf` must be `cap` bytes long | ||
| /// - `grow()` must either return null or a valid buffer of at least the requested buffer size | ||
| /// - Rust code must call `ICU4XCustomWriteable::flush()` before releasing to C | ||
| pub struct ICU4XCustomWriteable { |
There was a problem hiding this comment.
Suggestion: Let's bikeshed the name of this struct. Why did you put "Custom" in the name?
There was a problem hiding this comment.
Because I copied the name from the doc, I'm not particularly attached to it
Co-authored-by: Shane F. Carr <shane@unicode.org>
| /// - `flush()` and `grow()` will be passed `context` and the value should be valid for that | ||
| /// `context` may be null, however `flush()` and `grow()` must then be ready to receive it |
There was a problem hiding this comment.
Nit: This comment is not relevant for safety of the struct, because:
- The value in
contextis only read on the C side. - In Rust, it is already
unsafeto reference a pointer.
I would delete this line and explain it further in the docs for context below.
There was a problem hiding this comment.
I think it's still important to document what flush and grow expect.
Co-authored-by: Shane F. Carr <shane@unicode.org>
| buf, | ||
| len: 0, | ||
| // keep an extra byte in our pocket for the null terminator | ||
| cap: len - 1, |
There was a problem hiding this comment.
suggestion: To me, this feels more like example or test code, rather than something that is a core part of CustomWriteable. I'd prefer to see an implementation in C that lives as part of the example code.
There was a problem hiding this comment.
I think it's a common enough use case that it's good to ship a basic CustomWriteable so that it's possible to experiment with ICU4X without too much boilerplate.
Co-authored-by: Shane F. Carr <shane@unicode.org>
| /// - `buf` must be `cap` bytes long | ||
| /// - `grow()` must either return null or a valid buffer of at least the requested buffer size | ||
| /// - Rust code must call `ICU4XCustomWriteable::flush()` before releasing to C | ||
| pub struct ICU4XCustomWriteable { |
| /// FFI version of [`FixedDecimal::multiply_pow10()`]. See its docs for more details.ICU4XFixedDecimal | ||
| /// | ||
| /// Returns `true` if the multiplication was successful. | ||
| pub extern "C" fn icu4x_fixed_decimal_multiply_pow10( |
There was a problem hiding this comment.
Suggestion: Add negate() here as well, since it is the only way to get -0. Consider optionally adding .signum() as well. We should strive to have full API coverage.
There was a problem hiding this comment.
I feel like methods reading from FixedDecimal aren't really that necessary in our public API for now.
My attitude here is more go go gradually and add stuff as necessary. But I added negate()
Todo:
FixedDecimalFormat::formatand returning strings across the boundary (blocked on discussion)Fixes #663