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

Consider revising the return value of is_normalized_up_to to return head and tail instead of index #5217

Open
hsivonen opened this issue Jul 10, 2024 · 1 comment · May be fixed by #5932
Open

Consider revising the return value of is_normalized_up_to to return head and tail instead of index #5217

hsivonen opened this issue Jul 10, 2024 · 1 comment · May be fixed by #5932
Assignees
@hsivonen
Labels
2.0-breaking Changes that are breaking API changes C-collator Component: Collation, normalization
Milestone
ICU4X 2.0 Stretch...

Comments

@hsivonen
Copy link
Member

From #5216 (review):

thought: maybe is_normalized_up_to should return this tuple, because split_at will be an additional bounds check and a potential panic. The length can be recovered by doing .0.len() if that's needed.

That would probably imply a change to method naming, too.
@hsivonen hsivonen added C-collator Component: Collation, normalization 2.0-breaking Changes that are breaking API changes labels Jul 10, 2024
@hsivonen hsivonen mentioned this issue Jul 10, 2024
Merged
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Jul 23, 2024
@sffc sffc added this to icu4x 2.0 Jul 23, 2024
@sffc sffc moved this to Unclaimed for sprint in icu4x 2.0 Jul 23, 2024
@hsivonen hsivonen added the discuss-priority Discuss at the next ICU4X meeting label Dec 19, 2024
hsivonen added a commit to hsivonen/icu4x that referenced this issue Dec 19, 2024
@hsivonen
Introduce split_normalized and make is_normalized_up_to private in Rust
0d97a15 
Fixes unicode-org#5217
@hsivonen hsivonen linked a pull request Dec 19, 2024 that will close this issue
Open
@Manishearth
Copy link
Member

  • @hsivonen Should it use split_at? If we return the result of split_at from ICU4X, we'd need to use split_at_unchecked for it to make sense performance-wise
  • manishearth: we shouldn't panic if possible, let's split_at_checked with gigo behavior for broken stuff (perhaps return (empty, entire_string) or vice versa). We can optimize later if needed.
  • @sffc I think returning a pair of str is better. The caller of this API is likely going to want the string slices, and it's easy for them to recover the index
  • @sffc Would be nice to use split_at_checked but it is MSRV 1.80
  • manishearth: Rust 1.80 was released in July. It seems like 2.0 is a good time to increase the MSRV.
  • @hsivonen So then should is_normalized_up_to be public or private, in Rust and FFI?
  • @sffc The Rust fn should return the string slices and the FFI fn should return the index.

@Manishearth Manishearth removed the discuss-priority Discuss at the next ICU4X meeting label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hsivonen hsivonen

Labels
2.0-breaking Changes that are breaking API changes C-collator Component: Collation, normalization
Projects
icu4x 2.0
Status: Unclaimed for sprint
Milestone
ICU4X 2.0 Stretch ⟨P2⟩
Development

Successfully merging a pull request may close this issue.

Introduce split_normalized and make is_normalized_up_to private in Rust hsivonen/icu4x
3 participants
@hsivonen @sffc @Manishearth