-
Notifications
You must be signed in to change notification settings - Fork 183
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 methods to iterate exemplar characters #6079
base: main
Are you sure you want to change the base?
Add methods to iterate exemplar characters #6079
Conversation
ecf4338
to
e3cf37f
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.
Mostly looks good! Curious: what are you using ICU4X-JS for? So far we've been getting C++ users, excited to see some uptake in the JS backend!
impl StringIterator { | ||
/// Advance the iterator by one and return the next string, terminated with a null byte. | ||
/// If there are no more strings to be iterated, an empty string is returned. | ||
pub fn next(&mut self, write: &mut DiplomatWrite) { |
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.
issue: Diplomat supports iterators, I suggest tacking on #[diplomat::attr(auto, iterator)]
and having this return an Option<()>
.
We should probably make sure all other iterable APIs in icu_capi use iterators too.
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.
we shouldn't add a generic stringiterator type IMO, it's going to do expensive intermediate allocations.
/// An iterator over strings | ||
#[diplomat::opaque] | ||
pub struct StringIterator( | ||
pub Box<dyn Iterator<Item = String>>, |
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.
issue: this should reference a concrete type, and borrow from it
FnInStruct | ||
)] | ||
pub fn strings(&self) -> Box<StringIterator> { | ||
let strings = self.0.as_borrowed().strings().iter().map(|s| s.to_string()).collect::<Vec<_>>(); |
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.
issue: these two should not collect, Diplomat can return borrows. This should return a Box<ExemplarCharactersStringIterator<'a>>
Ideally there's no dynamic dispatch, if we need concrete iterator types in ICU4X (where we have `impl Trait) we should add them.
Adds public
code_point_ranges
andstrings
methods toExemplarCharacters
to allow iterating through the single- and multi-character entries, respectively:code_point_ranges
returns aCodePointRangeIterator
(existing struct)strings
returns aStringIterator
, a new struct that iterates via passing aDiplomatWrite
buffer to itsnext
method, writing a null-char-terminated string forSome(string)
and an empty string forNone
(thus differentiatingSome("") -> "\0"
fromNone -> ""
, though in practiceSome("")
should never occur for exemplar chars). Not sure if there's a better way of iterating strings across the FFI boundary.Thus for
hu
locale andMain
set:I also added a tab to the WASM demo to illustrate how these can be used from JS/TS.