feat: Implement uloc_openAvailableByType() #295
Conversation
clydegerber
commented
Oct 31, 2023
•
clydegerber
commented
- Change the signature of ULoc::get_available() to more closely match the ICU4C uloc_getAvailable() function signature.
- Add the function ULoc::get_available_locales() to return an (uncached) collection of available locales.
- Add the function ULoc::get_available_locales_by_type() to return a collection of available locales for the specified type.
- Correct some configuration tags that used the plural 'features' incorrectly and were not functioning correctly as a result.
rust_icu_uloc/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Returns a vector of locales of the requested type. | ||
| pub fn get_available_by_type(locale_type: ULocAvailableType) -> &'static Vec<ULoc> { |
There was a problem hiding this comment.
It seems that more of this functionality will need to be pushed down to ICU.
There was a problem hiding this comment.
Are you okay with adding the static vectors to this library? The underlying assumption is that the ICU library is loaded once so that it is safe to cache these static results. If dynamically opening and closing the ICU library underneath this library is required then they may not be a good idea.
There was a problem hiding this comment.
Indeed, assuming static data is not going to work. ICU data can be loaded and unloaded.
In other places we've used this was to make it into iterator using Enumerate, but it seems that this particular API doesn't follow that pattern.
It might be OK to recompute the vector on every call, and have the user cache the results instead if they need to.
There was a problem hiding this comment.
Removed the caching and recomputing on every call - have a look.
clydegerber
changed the title
|
Looks file test failures against the 6.3 ICU version were due to the uloc_openAvailableByType() API not being available until 6.5. I've attempted to add a feature for the 6.5 version and make the new API's dependent on that feature. I would appreciate a thorough review of what I've done, as I simply followed the examples for the other version features. In the course of this I noticed an issue in my previous commits that I'm correcting as well. |
|
Will do, but please allow a day or so.
F
…On Fri, Nov 3, 2023 at 10:54 AM clydegerber ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In rust_icu_uloc/src/lib.rs
<#295 (comment)>:
> @@ -385,6 +385,36 @@ impl ULoc {
})
}
+ /// Implements `uloc_openAvailableByType`.
+ pub fn open_available_by_type(locale_type: ULocAvailableType) -> Result<Enumeration, common::Error> {
+ let mut status = common::Error::OK_CODE;
+ unsafe {
+ let iter = Enumeration::from_raw_parts(None, versioned_function!(uloc_openAvailableByType)(locale_type, &mut status));
+ common::Error::ok_or_warning(status)?;
+ Ok(iter)
+ }
+ }
+
+ /// Returns a vector of locales of the requested type.
+ pub fn get_available_by_type(locale_type: ULocAvailableType) -> &'static Vec<ULoc> {
Removed the caching and recomputing on every call - have a look.
—
Reply to this email directly, view it on GitHub
<#295 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB4GMEG5KSQFH6BTD2JFBTYCUVWFAVCNFSM6AAAAAA6YLV3R2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJTGI2DMOBYGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
clydegerber
force-pushed
the
available_by_type
branch
from
0ac4b80 to
c8451a7
Compare
clydegerber
changed the title
rust_icu_uloc/src/lib.rs
Outdated
| } | ||
|
|
||
| /// Implements `uloc_openAvailableByType`. | ||
| #[cfg(feature = "icu_version_65_plus")] |
There was a problem hiding this comment.
Since we no longer support v65 anyways, you can use icu_version_67_plus as well, without the need to introduce a new (onerous) feature.
rust_icu_uloc/src/lib.rs
Outdated
| }) | ||
| pub fn get_available(n: i32) -> Result<ULoc, common::Error> { | ||
| if (0 > n) || (n >= Self::count_available()) { | ||
| panic!("n is negative or greater than or equal to the value returned from count_available()"); |
There was a problem hiding this comment.
You should probably also print the value of n, to allow for self-evident error message here.
rust_icu_uloc/src/lib.rs
Outdated
| let mut vec = Vec::with_capacity(count as usize); | ||
| let mut index: i32 = 0; | ||
| while index < count { | ||
| let label = unsafe { ffi::CStr::from_ptr(versioned_function!(uloc_getAvailable)(index)).to_str().unwrap() }; |
There was a problem hiding this comment.
May want to check for the ptr value here - sorry I missed this on the first go.
rust_icu_uloc/src/lib.rs
Outdated
| if (0 > n) || (n >= Self::count_available()) { | ||
| panic!("n is negative or greater than or equal to the value returned from count_available()"); | ||
| } | ||
| let label = unsafe { ffi::CStr::from_ptr(versioned_function!(uloc_getAvailable)(n)).to_str().unwrap() }; |
There was a problem hiding this comment.
May want to check for the ptr value here - sorry I missed this on the first go.
|
Sorry - I had these comments pending for a while but didn't send them out. |
…collection of locales for the speficied type.
…le_by_type() API for that feature only since uloc_getAvailableByType() is not stable prior to 6.5. Correct some feature configurations which incorrectly used the plural 'features' and were thus not operating as desired.
- Add a new feature for ICU4C version 6.5 and above in which the new implementation will be available. - Change the signature of ULoc::get_available() to more closely match the ICU4C uloc_getAvailable() function signature. - Add the function ULoc::get_available_locales() to return an (uncached) collection of available locales. - Add the function ULoc::get_available_locales_by_type() to return a collection of available locales for the specified type. - Correct some configuration tags that used the plural 'features' incorrectly and were not functioning correctly as a result.
- Remove icu_version_65_plus feature. - Use icu_version_67_plus for determining support for uloc_getAvailableByType. - Ouput the invalid index value supplied to get_available() - Check for null pointer returned from uloc_getAvailable()
- Change the signature of ULoc::get_available() to more closely match the ICU4C uloc_getAvailable() function signature. - Add the function ULoc::get_available_locales() to return an (uncached) collection of available locales. - Add the function ULoc::get_available_locales_by_type() to return a collection of available locales for the specified type. - Correct some configuration tags that used the plural 'features' incorrectly and were not functioning correctly as a result.
clydegerber
force-pushed
the
available_by_type
branch
from
c8451a7 to
5b63fed
Compare
