Skip to content
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

Add FFI for FixedDecimalFormat #680

Merged
merged 25 commits into from
May 4, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions components/capi/examples/fixeddecimal/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ int main() {

ICU4XCustomWriteable write = icu4x_simple_writeable(output, 40);

bool success = icu4x_fixed_decimal_format_format(fdf, decimal, &write);
bool success = icu4x_fixed_decimal_format_write(fdf, decimal, &write);
if (!success) {
printf("Failed to write result of FixedDecimalFormat::format to string.\n");
return 1;
}
printf("Output is %s\n", output);

char* expected = "১০,০০,০০৭";
const char* expected = u8"১০,০০,০০৭";
if (strcmp(output, expected) != 0) {
printf("Output does not match expected output!\n");
return 1;
Expand Down
2 changes: 1 addition & 1 deletion components/capi/include/custom_writeable.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <stdbool.h>

typedef struct {
void* data;
void* context;
char* buf;
size_t len;
size_t cap;
Expand Down
2 changes: 1 addition & 1 deletion components/capi/include/decimal.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ typedef struct {

ICU4XCreateFixedDecimalFormatResult icu4x_fixed_decimal_format_create(const ICU4XLocale* locale, const ICU4XDataProvider* provider, ICU4XFixedDecimalFormatOptions options);

bool icu4x_fixed_decimal_format_format(const ICU4XFixedDecimalFormat* fdf, const ICU4XFixedDecimal* value, ICU4XCustomWriteable* write);
bool icu4x_fixed_decimal_format_write(const ICU4XFixedDecimalFormat* fdf, const ICU4XFixedDecimal* value, ICU4XCustomWriteable* write);
void icu4x_fixed_decimal_format_destroy(ICU4XFixedDecimalFormat* fdf);


Expand Down
39 changes: 24 additions & 15 deletions components/capi/src/custom_writeable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,31 @@ use std::{fmt, ptr};
///
/// [`icu4x_simple_writeable()`] can be used to write to a fixed-size char buffer.
///
/// May be extended in the future to support further invariants
///
/// ICU4XCustomWriteable will not perform any cleanup on `context` or `buf`, these are logically
/// "borrows" from the FFI side.
///
/// # Safety invariants:
/// - `flush()` and `grow()` will be passed `data` and the value should be valid for that
/// `data` may be null, however `flush()` and `grow()` must then be ready to receive it
/// - `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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This comment is not relevant for safety of the struct, because:

  1. The value in context is only read on the C side.
  2. In Rust, it is already unsafe to reference a pointer.

I would delete this line and explain it further in the docs for context below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still important to document what flush and grow expect.

/// - `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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Let's bikeshed the name of this struct. Why did you put "Custom" in the name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I copied the name from the doc, I'm not particularly attached to it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just ICU4XWriteable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me!

/// Pointer to the actual object. While we're writing, we will write
/// directly to `buf` without updating `data`'s state.
data: *mut c_void,
/// The buffer to write to
/// directly to `buf` without updating `context`'s state, this pointer exists so that
/// `grow()` and `flush()` can get access to the full object on the foreign side.
///
/// This can be null if the
context: *mut c_void,
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
/// The buffer to write directly to
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
buf: *mut u8,
/// The current filled size of the buffer
len: usize,
/// The current capacity of the buffer
cap: usize,
/// Called by Rust code when it is done writing, updating `data`
/// Called by Rust code when it is done writing, updating `context`
/// with the new length
flush: extern "C" fn(*mut c_void, usize),
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
/// Called by Rust when it needs more capacity, passing
Expand All @@ -41,21 +49,21 @@ pub struct ICU4XCustomWriteable {
impl ICU4XCustomWriteable {
/// Call this function before releasing the buffer to C
pub fn flush(&mut self) {
(self.flush)(self.data, self.len);
(self.flush)(self.context, self.len);
}
}
impl fmt::Write for ICU4XCustomWriteable {
fn write_str(&mut self, s: &str) -> Result<(), fmt::Error> {
let mut needed_len = self.len + s.len();
let needed_len = self.len + s.len();
if needed_len > self.cap {
let newbuf = (self.grow)(self.data, &mut needed_len);
let mut new_cap = needed_len;
let newbuf = (self.grow)(self.context, &mut new_cap);
if newbuf.is_null() {
return Err(fmt::Error);
}
self.cap = needed_len;
self.cap = new_cap;
self.buf = newbuf;
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
}
let needed_len = self.len + s.len();
debug_assert!(needed_len <= self.cap);
unsafe {
ptr::copy_nonoverlapping(
Expand All @@ -74,19 +82,20 @@ impl fmt::Write for ICU4XCustomWriteable {
/// Once done, this will append a null terminator to the written string.
#[no_mangle]
pub unsafe extern "C" fn icu4x_simple_writeable(buf: *mut u8, len: usize) -> ICU4XCustomWriteable {
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
extern "C" fn grow(_data: *mut c_void, _cap: *mut usize) -> *mut u8 {
extern "C" fn grow(_context: *mut c_void, _cap: *mut usize) -> *mut u8 {
ptr::null_mut()
}
extern "C" fn flush(data: *mut c_void, size: usize) {
extern "C" fn flush(context: *mut c_void, size: usize) {
unsafe {
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
let buf = data as *mut u8;
let buf = context as *mut u8;
ptr::write(buf.offset(size as isize), 0)
Manishearth marked this conversation as resolved.
Show resolved Hide resolved
}
}
ICU4XCustomWriteable {
data: buf as *mut c_void,
context: buf as *mut c_void,
buf,
len: 0,
// keep an extra byte in our pocket for the null terminator
cap: len - 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

flush,
grow,
Expand Down
2 changes: 1 addition & 1 deletion components/capi/src/decimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub extern "C" fn icu4x_fixed_decimal_format_create<'d>(
/// FFI version of [`FixedDecimalFormat::format()`]. See its docs for more details.
///
/// Returns `false` when there were errors writing to `write`
pub extern "C" fn icu4x_fixed_decimal_format_format(
pub extern "C" fn icu4x_fixed_decimal_format_write(
fdf: &ICU4XFixedDecimalFormat<'_>,
value: &ICU4XFixedDecimal,
write: &mut ICU4XCustomWriteable,
Expand Down