Add Copy indexing method for timezone lookup #6902
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
utils/zoneinfo64/src/lib.rs
Outdated
| @@ -98,6 +98,10 @@ impl Debug for TzZoneData<'_> { | |||
| } | |||
| } | |||
|
|
|||
| /// A way to index into ZoneInfo64 without performing string comparisons each time | |||
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | |||
| pub struct ZoneIndex(usize); | |||
| /// a Copy type. | ||
| pub fn get_indexed(&'a self, index: ZoneIndex) -> Option<Zone<'a>> { | ||
| let idx = index.0; | ||
|
|
||
| #[expect(clippy::indexing_slicing)] // binary search |
There was a problem hiding this comment.
"binary search" is not correct anymore. the index type needs to state its invariants
There was a problem hiding this comment.
I'm not sure I'm aligned on the goal here. The current get returns a Copy type already, so what exactly is the issue? The lifetime? This PR has the issue that the ZoneIndex is not tied to the ZoneInfo64 struct, so it could be an incorrect index. This can be fixed by borrowing the ZoneInfo64, so the lifetime is kind of required. Or is it the size? We could store only the index inside Zone and look up things when we need them, this would essentially make it the same as ZoneIndex (but with the reference to the ZoneInfo64).
In temporal_rs we'd like to be generic over returned type without actually using generics if possible. Having usize indices passed around would be most efficient. The lifetime is definitely an issue, temporal_rs does not want to pass around I think our options are largely to return a usize or scrap the copy-TimeZone plan altogether. |
|
cc @nekevss |
utils/zoneinfo64/src/lib.rs
Outdated
| @@ -100,7 +100,7 @@ impl Debug for TzZoneData<'_> { | |||
|
|
|||
| /// A way to index into ZoneInfo64 without performing string comparisons each time | |||
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] | |||
| pub struct ZoneIndex(usize); | |||
| pub struct ZoneIndex(pub usize); | |||
There was a problem hiding this comment.
there's little point for a newtype if the single field is public.
There was a problem hiding this comment.
I'm okay with just having a usize; the newtype helps make signatures clear.
Conclusion: Accept #6903, with u16 getters/setters Agreed: @Manishearth @sffc @robertbastian |
|
Closed in favor of #6903 |
Alternative to #6902 This defers the lookups in `zones`, `names`, `rules`, and `regions` from the constructor, only storing the index in the `Zone` struct. This will cost an additional lookup at the beginning of each zone operation, but these are fairly lookup-heavy already anyway. It also saves the lookups for name and region, which are probably only needed very rarely. In exchange, the size of `Zone` gets significantly decreased. Also adds a `RawZone` abstraction which can be GIGO converted to and from `Zone`, with the invariant being that the same `ZoneInfo64` instance is used throughout. Users like temporal can combine this with a static `LazyLock<ZoneInfo64>` to provide a safe wrapper with minimal size.
This will be useful for the temporal_rs TimeZoneProvider design.