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 GenericPatternItemULE::as_pattern_item_ule #4497

Merged
merged 3 commits into from
Dec 27, 2023

Conversation

sffc
Copy link
Member

@sffc sffc commented Dec 27, 2023

Smaller change pulled off from #4415

This includes a bit of unsafe code and takes advantage of the fact that currently GenericPatternItemULE and PatternItemULE have the same repr if they are representing a code point. I added a debug assertion to ensure that this invariant remains true.

@sffc sffc requested review from Manishearth and removed request for nordzilla December 27, 2023 03:17
Manishearth
Manishearth previously approved these changes Dec 27, 2023
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

code lgtm

I'd like the repr docs for both of them to include a mention of this soft invariant ("if changing the repr, consider making sure this transmute is still possible") so that when we ever go to change this there are early warnings.

r=me with that, feel free to merge beforehand

Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

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

ain't gonna block on it, but the (ab)use of Result to indicate enum is unintuitive for me.
Result::Err indicates not just the value but also a misalignment with state expectations. In this case Err is a perfectly expected and reasonable state.

@sffc
Copy link
Member Author

sffc commented Dec 27, 2023

ain't gonna block on it, but the (ab)use of Result to indicate enum is unintuitive for me. Result::Err indicates not just the value but also a misalignment with state expectations. In this case Err is a perfectly expected and reasonable state.

I argue that it is a valid use because the fn as_pattern_item_ule either succeeds or fails at the conversion. I suppose try_as_pattern_item_ule might be a more appropriate name. But, this is a private fn.

@sffc sffc merged commit 3f50b42 into unicode-org:main Dec 27, 2023
29 checks passed
@sffc sffc deleted the as-pattern-item-ule branch December 27, 2023 17:31
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