Make FFI DataProvider only support BufferProvider #5895
Conversation
sffc
changed the title
|
|
Manishearth
force-pushed
the
provider-capi-simpler
branch
2 times, most recently
from
44a961c to
9bfe85f
Compare
|
finally all of the CFGs seem fixed. that took way too long, but with this landed I believe we're done with the tedious part of our FFI 2.0 fixes |
Manishearth
force-pushed
the
provider-capi-simpler
branch
from
9bfe85f to
1ea1fad
Compare
Manishearth
force-pushed
the
provider-capi-simpler
branch
from
1ea1fad to
f8bc3fd
Compare
| @@ -30,6 +30,12 @@ pub enum DateTimeFormatterLoadError { | |||
| Data(DataError), | |||
| } | |||
|
|
|||
| impl From<DataError> for DateTimeFormatterLoadError { | |||
There was a problem hiding this comment.
I'm okay with this, but @robertbastian pushed back on adding the From impls for the fine-grained error types in this crate.
There was a problem hiding this comment.
hm. we kind of need it to use the closure but we could also not use the closure and revive the macro instead for these few call sites.
There was a problem hiding this comment.
hm. we kind of need it to use the closure but we could also not use the closure and revive the macro instead for these few call sites.
| Ok(Box::new(Calendar(Arc::new(provider.call_constructor( | ||
| |provider| { | ||
| icu_calendar::AnyCalendar::try_new_for_kind_with_buffer_provider( | ||
| provider, | ||
| kind.into(), | ||
| ) | ||
| }, | ||
| )?)))) |
There was a problem hiding this comment.
I don't specifically recall a discussion on changing the macro to a closure. It seems like it would be harder in the future to change the behavior back to supporting try_new_unstable and similar. But it's fine.
There was a problem hiding this comment.
Nope, this wasn't discussed before: this PR was the discussion opportunity, and I can remove the last N commits if we don't want this.
But it should be easy enough to bring back when we need it.
I tend to consider macro use to be suboptimal, so if they can be removed without sacrificing anything I try to remove them. Here the complexity has not increased and the main downside is wanting to refactor again in the medium term.
| icu_experimental::units::converter_factory::ConverterFactory::try_new_with_buffer_provider, | ||
| provider, | ||
| )?))) | ||
| Ok(Box::new(UnitsConverterFactory(provider.call_constructor(icu_experimental::units::converter_factory::ConverterFactory::try_new_with_buffer_provider)?))) |
There was a problem hiding this comment.
Thought: lines like this look to me like they should be formatted.
|
Merging, I will improve docs in a new PR. We can still discuss if we really want the closure and if the From impl is a problem. |
Fixes #4639
I still plan to do docs for this, please review the actual changes first.
"refactor" and "move usages to closures" are commits that are largely mechanical.
In the last two commits I attempt to get rid of the macro in favor of a closure based situation. Datetime uses a custom error so it's weird.