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 CPT::try_alloc_map_value #3207

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Add CPT::try_alloc_map_value #3207

merged 1 commit into from
Mar 20, 2023

Conversation

sffc
Copy link
Member

@sffc sffc commented Mar 19, 2023

@sffc sffc requested a review from echeran as a code owner March 19, 2023 06:29
/// let planes_trie_u8: CodePointTrie<u8> = planes::get_planes_trie();
/// let planes_trie_u16: CodePointTrie<u16> = planes_trie_u8
/// .try_alloc_map_value(TryFrom::try_from)
/// .expect("infallible");
Copy link
Member Author

Choose a reason for hiding this comment

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

If rust-lang/rust#61695 ever gets stabilized then we won't need to .expect("infallible") any more

Copy link
Contributor

@echeran echeran left a comment

Choose a reason for hiding this comment

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

LGTM. I tested this out locally on top of the PR that motivated this, and I can successfully run cargo make testdata when I previously couldn't.

///
/// let planes_trie_u8: CodePointTrie<u8> = planes::get_planes_trie();
/// let planes_trie_u16: CodePointTrie<u16> = planes_trie_u8
/// .try_alloc_map_value(TryFrom::try_from)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: use different .map() function to make type width expansion more obvious

Could you replace the function with the cube function |x| x * x * x? (the square function would suffice since the 17th plane => 16 => 16^2 > u8:MAX, but x^3 is more obvious)

Copy link
Member

Choose a reason for hiding this comment

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

I think the use case of widening the trie is also valid, so we don't need a special map.

Comment on lines +451 to +460
/// use icu_collections::codepointtrie::CodePointTrie;
/// use icu_collections::codepointtrie::planes;
/// use core::convert::Infallible;
///
/// let planes_trie_u8: CodePointTrie<u8> = planes::get_planes_trie();
/// let planes_trie_u16: CodePointTrie<u16> = planes_trie_u8
/// .try_alloc_map_value(TryFrom::try_from)
/// .expect("infallible");
///
/// assert_eq!(planes_trie_u16.get32(0x30000), 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

totally optional. this is what i was thinking of in the previous comment. i wanted to show a x^3 function, but i wasn't sure what the Error type would look like for that.

Suggested change
/// use icu_collections::codepointtrie::CodePointTrie;
/// use icu_collections::codepointtrie::planes;
/// use core::convert::Infallible;
///
/// let planes_trie_u8: CodePointTrie<u8> = planes::get_planes_trie();
/// let planes_trie_u16: CodePointTrie<u16> = planes_trie_u8
/// .try_alloc_map_value(TryFrom::try_from)
/// .expect("infallible");
///
/// assert_eq!(planes_trie_u16.get32(0x30000), 3);
/// use icu_collections::codepointtrie::CodePointTrie;
/// use icu_collections::codepointtrie::planes;
/// use core::convert::Infallible;
///
/// fn try_square(x: u8) -> Result<u16, Infallible> {
/// let y = x as u16;
/// Ok(y * y)
/// }
///
/// let planes_trie_u8: CodePointTrie<u8> = planes::get_planes_trie();
/// let planes_trie_u16: CodePointTrie<u16> = planes_trie_u8
/// .try_alloc_map_value(try_square)
/// .expect("infallible");
///
/// assert_eq!(planes_trie_u8.get32(0x10_0000), 16);
/// assert_eq!(planes_trie_u16.get32(0x10_0000), 256);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep; actually the square function could have an error condition in the case of overflow. It could be written with checked_mul.

I'm going to merge the PR as-is but may follow up with this.

P: TrieValue,
{
let error_converted = f(self.error_value)?;
let converted_data = self.data.iter().map(f).collect::<Result<ZeroVec<P>, E>>()?;
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize transforming CPTs was so straightforward!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, for CodePointTrie it is easy but not CharsTrie :)

@sffc sffc merged commit ec0763a into unicode-org:main Mar 20, 2023
@sffc sffc deleted the cpt-converter branch March 20, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants