Making icu_locid_macros into consts
#1631
Conversation
robertbastian
requested review from
Manishearth,
sffc,
a team,
nordzilla,
zbraniecki,
nciric,
dminor and
gregtatum
as code owners
robertbastian
removed request for
a team,
dminor,
sffc,
gregtatum,
Manishearth,
nciric and
nordzilla
robertbastian
changed the title
icu_locid_macros into consts
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
There was a problem hiding this comment.
Looks good from my end! Please wait for @zbraniecki's review.
There was a problem hiding this comment.
overall it looks great! Thank you!
I am sad that const limitations make us have to sacrifice readability for const compatibility, but I believe it is a right tradeoff.
We can revisit and refactor the code when Rust const ergonomics are complete (? would help a lot).
I left a couple notes, but my two overarching ones are:
- Please, do not forgo the value of runtime parsing. icu_locid should be optimized for scenarios where code receives a string and needs to parse it at runtime. It's a very very hot loop.
- It would be great to find a good place to document the fact that we're giving up on readability to get consts and that this code should be refactored back once Rust const ergonomics are in place. Maybe as a comment over
LanguageIdentifiermain parser function?
We haven't done this in other places either, I'm thinking we can assume that if it's a const function it's clear that we don't get |
| @@ -71,6 +71,7 @@ mod helpers; | |||
| pub mod extensions; | |||
| mod langid; | |||
| mod locale; | |||
| pub mod macros; | |||
There was a problem hiding this comment.
Nit: Does it need to be pub? The macros are top-level via #[macro_export]
| parse_language_identifier_from_iter(&mut iter, mode) | ||
| } | ||
|
|
||
| pub const fn parse_language_identifier_without_variants_from_iter( |
There was a problem hiding this comment.
Note: I didn't review the files in this module in detail; assuming Zibi did that
Fixes #348