Introduce zoneinfo64 crate
#6726
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 is currently in preview and 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 to provide feedback. |
robertbastian
force-pushed
the
zi
branch
5 times, most recently
from
bab8df8 to
0e8f3d7
Compare
robertbastian
changed the title
zoneinfo64.res parsing example with actual logiczoneinfo64 crate
|
|
||
| #[derive(Debug, Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct TzZoneDataRaw<'a> { |
There was a problem hiding this comment.
nit: this is a lot of stuff living inside a function, I think we should just have a serde or parse module that has all of this
| @@ -65,7 +65,11 @@ struct ResourceTreeDeserializer<'de> { | |||
| impl<'de> ResourceTreeDeserializer<'de> { | |||
| /// Creates a new deserializer from the header and index of the resource | |||
| /// bundle. | |||
| fn from_bytes(input: &'de [u8]) -> Result<Self, BinaryDeserializerError> { | |||
| fn from_bytes(input: &'de [u32]) -> Result<Self, BinaryDeserializerError> { | |||
There was a problem hiding this comment.
question: why is this change needed? Won't this make it harder to deserialize from a file (since that gives you a &[u8], though it's usually aligned)
There was a problem hiding this comment.
resb files are padded, internal &[u32]s are 4-aligned from the start, and that's only useful if the start is 4-aligned. If we didn't do this we'd have to solve the whole alignment problem that we solved with ULE again (can't use ULE because these files are native endian).
If you need to read from a file you can create an aligned buffer and read into that. Alternatively, mmap guarantees to align to at least the largest native integer.
There was a problem hiding this comment.
you can create an aligned buffer and read into that
Hmm. I guess that somewhat works.
If we didn't do this we'd have to solve the whole alignment problem that we solved with ULE again (can't use ULE because these files are native endian).
Honestly I'd slightly prefer that, we'd just need to create u32Unaligned and operate on it. But doesn't have to be fixed now.
| use serde::de::*; | ||
|
|
||
| /// TODO | ||
| pub fn option_utf_16<'de, D: Deserializer<'de>>( |
There was a problem hiding this comment.
nit: for this file I'd love to see comments describing the bit layout of each of the deserialized formats
There was a problem hiding this comment.
I don't know the bit layout. resb gives me an Option<&[u16]>, this maps that to an Option<&PotentialUtf16>
| if bytes.as_ptr().align_offset(core::mem::align_of::<T>()) != 0 | ||
| && bytes.len() % core::mem::size_of::<T>() != 0 | ||
| { | ||
| return Err(E::custom("Wrong length or align")); |
There was a problem hiding this comment.
question: how do we know this doesn't get triggered?
There was a problem hiding this comment.
Because we 4-aligned the file and resb guarantees the rest.
There was a problem hiding this comment.
Yeah, I'd prefer to not have to rely on that, but that's fine for now.
There was a problem hiding this comment.
That's why I'm checking this and not just casting
There was a problem hiding this comment.
I meant for correctness, not safety: I'd still prefer to not have to worry about this code throwing errors.
There was a problem hiding this comment.
How do you expect deserialisation code to never throw errors?
serde can't borrow &[u32] and that's the only API resb has right now.
There was a problem hiding this comment.
Right, I'm suggesting this code be made to work on &[u8] (which it turns into unaligned endianness-dependent u32s).
I'm not saying we do that now, but if this operated on &[u8] then the rest of this trickiness would go away.
There was a problem hiding this comment.
It will introduce a lot of trickiness in other places. Requiring the blob to be 4-aligned lets us operate on aligned values, which makes the code a lot easier to write, and probably more performant as well. We should take advantage of the file being padded.
No description provided.