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

Re-write locale macros as const functions #348

Closed
sffc opened this issue Oct 14, 2020 · 2 comments · Fixed by #1631
Closed

Re-write locale macros as const functions #348

sffc opened this issue Oct 14, 2020 · 2 comments · Fixed by #1631
Assignees
Labels
C-locale Component: Locale identifiers, BCP47 good first issue Good for newcomers T-techdebt Type: ICU4X code health and tech debt

Comments

@sffc
Copy link
Member

sffc commented Oct 14, 2020

Proc macros are clunky and error-prone. Problems include:

  1. You cannot depend on icu_locid_macros by itself; you need to include either icu or icu_locid
  2. They are heavier than macro_rules! and const fn, since the compiler needs to link and run them as a separate binary
  3. The macros crate needs to be published separately on crates.io
  4. Pulls in a nontrivial proc-macro-crate dependency in Cargo.toml

We are close to being able to write the locale macros as const functions.

I previous said in #310 (comment):

Okay, I also do not see any sign of string/slice indexing operations. I saw it suggested in rust-lang/rust#52000 that this could change (allowing for loops), but there doesn't seem to be a lot of movement on the idea of the fundamental indexing operation in const functions.

That being said, since we can pretty trivially implement things like TinyStr4::is_ascii_alphabetic() as const functions, we could limit the locale macro to be along the lines of

langid!("en")
langid!("en", "US")
langid!("en", "Latn", "US")

We can validate the subtags, but we can't parse the "en-Latn-US" syntax inside the const function in any clean way. (I guess you could convert the string to a TinyStr16 and do bitwise operations on that...)

I think we should consider this route as a more future-proof solution to this problem.

@sffc sffc added the discuss Discuss at a future ICU4X-SC meeting label Oct 14, 2020
@zbraniecki zbraniecki added the help wanted Issue needs an assignee label Oct 15, 2020
@sffc sffc added C-locale Component: Locale identifiers, BCP47 question Unresolved questions; type unclear and removed help wanted Issue needs an assignee labels Oct 20, 2020
@zbraniecki
Copy link
Member

There's also a pre-RFC to get proc_macros inside a crate - https://internals.rust-lang.org/t/pre-rfc-add-macros-target-to-cargo-manifest/12428/4

@sffc sffc added backlog T-bug Type: Bad behavior, security, privacy T-techdebt Type: ICU4X code health and tech debt help wanted Issue needs an assignee and removed discuss Discuss at a future ICU4X-SC meeting question Unresolved questions; type unclear T-bug Type: Bad behavior, security, privacy labels Oct 30, 2020
@sffc sffc added blocked A dependency must be resolved before this is actionable good first issue Good for newcomers labels Jun 10, 2021
@sffc
Copy link
Member Author

sffc commented Jun 10, 2021

Blocked on upstream Rust const function RFCs including const panic.

@robertbastian robertbastian self-assigned this Feb 21, 2022
@robertbastian robertbastian linked a pull request Feb 23, 2022 that will close this issue
@sffc sffc removed the blocked A dependency must be resolved before this is actionable label Feb 23, 2022
@sffc sffc added this to the 2022 Q1 0.6 Sprint C milestone Feb 23, 2022
@sffc sffc removed help wanted Issue needs an assignee backlog labels Feb 23, 2022
@sffc sffc removed the v1 label Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-locale Component: Locale identifiers, BCP47 good first issue Good for newcomers T-techdebt Type: ICU4X code health and tech debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants