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

Migrate data structs to icu_pattern in 2.0 #5256

Closed
sffc opened this issue Jul 17, 2024 · 4 comments
Closed

Migrate data structs to icu_pattern in 2.0 #5256

sffc opened this issue Jul 17, 2024 · 4 comments
Labels
2.0-breaking Changes that are breaking API changes

Comments

@sffc
Copy link
Member

sffc commented Jul 17, 2024

Now that we have icu_pattern, we should migrate more components to use it where possible. Candidates:

  1. icu_decimal (related: Reduce stack size of DecimalSymbolsV1 #4437)
  2. icu_list
  3. Anything sitting around in icu_experimental that hasn't been migrated yet (not a 2.0 blocker)
  4. Maybe icu_datetime but that will require more surgery and might not be worth it
  5. Others?

CC @robertbastian

@sffc sffc added the 2.0-breaking Changes that are breaking API changes label Jul 17, 2024
@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Jul 17, 2024
@sffc sffc added this to icu4x 2.0 Jul 17, 2024
@sffc sffc moved this to Unclaimed for sprint in icu4x 2.0 Jul 17, 2024
@robertbastian
Copy link
Member

icu_list has a couple of invariants that are not available on icu_pattern, specifically that {0} and {1} have to appear in that order, and that nothing can precede the {0} (end and middle patterns) or follow the {1} (start and middle patterns). These invariants allow taking substrings of the patterns to be stitched together, instead of creating a massive tree of Writeables, which I think is more efficient.

@sffc
Copy link
Member Author

sffc commented Aug 20, 2024

Nested writeables can usually be composed in such a way that it should be equivalent to a loop when unrolled, but I agree it can be tricky to get to that point.

The list data struct could apply the additional invariants during deserialization.

The main thing we want for 2.0 though is to evaluate the cases and make an educated engineering decision that certain things are too much work / infeasible / lacking in ROI. I'm okay making such a determination for ListFormatter data.

@robertbastian
Copy link
Member

The list data struct could apply the additional invariants during deserialization.

Then I'm basically writing a new pattern type. As I already have a pattern type I don't see what the upside would be.

robertbastian added a commit that referenced this issue Aug 20, 2024
This is smaller than optional fields or a `ZeroMap`. Lookup time is
negligible as it will contain at most 5 items, usually just 1 or 2.

#5139, #5256
@sffc
Copy link
Member Author

sffc commented Sep 17, 2024

Closing this because all work is tracked elsewhere

@sffc sffc closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2024
@github-project-automation github-project-automation bot moved this from Unclaimed for sprint to Done in icu4x 2.0 Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0-breaking Changes that are breaking API changes
Projects
Status: Done
Development

No branches or pull requests

2 participants