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.

Sign up for GitHub

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

Jump to bottom

Fail with length error in PlainOldULE, CharULE, etc. #1144

Closed
Tracked by #1082
sffc opened this issue Oct 3, 2021 · 1 comment · Fixed by #1147
Closed
Tracked by #1082

Fail with length error in PlainOldULE, CharULE, etc. #1144

sffc opened this issue Oct 3, 2021 · 1 comment · Fixed by #1147
Assignees
@Manishearth
Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-bug Type: Bad behavior, security, privacy
Milestone
ICU4X 0.4

Comments

@sffc
Copy link
Member

sffc commented Oct 3, 2021 •
edited
Loading

In these ULEs, we are not currently returning an error for ULE as the safety docs proposed in #1121 insist that we do. This is a bug that we need to fix now.

Question: What error type should we return in these two cases? I had proposed UleError with a special variant for length errors, but this was voted down in #1121. Currently, PlainOldULE is returning Infallible, and CharULE is returning CharTryFromError; clearly both of those will have to change.

Also add tests for this.

CC @Manishearth
@sffc sffc added T-bug Type: Bad behavior, security, privacy C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) labels Oct 3, 2021
@sffc sffc added this to the ICU4X 0.4 milestone Oct 3, 2021
@sffc sffc mentioned this issue Oct 3, 2021
Merged
@Manishearth Manishearth mentioned this issue Oct 4, 2021
Closed
29 tasks
@Manishearth
Copy link
Member

Manishearth commented Oct 4, 2021 •
edited
Loading

I actually do think UleError is a good idea, I just didn't want the trait to automatically split out this check into a different method. We should probably introduce such an error type that people can use manually.

@Manishearth Manishearth mentioned this issue Oct 4, 2021
Merged
@Manishearth Manishearth self-assigned this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Manishearth Manishearth

Labels
C-data-infra Component: provider, datagen, fallback, adapters S-small Size: One afternoon (small bug fix or enhancement) T-bug Type: Bad behavior, security, privacy
Projects
None yet
Milestone
ICU4X 0.4
Development

Successfully merging a pull request may close this issue.

Add UleError type; use for PlainOldULE and char conversions Manishearth/icu4x
2 participants
@sffc @Manishearth