reduce reliance on alloc crate in src/tz/posix.rs
#179
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is mostly just removing some dead code, and in one case, finishing a refactoring to `SignedDuration` in `src/tz/posix.rs`.
As part of working toward #168, this commit puts POSIX time zone abbreviations inline instead of on the heap. We can generally rely on time zone abbreviations to be short, so this is "fine." This is obviously more annoying than just using `Box<str>`, which explains why I didn't do it this way initially. But it seems like a no-std no-alloc mode is a good idea, so let's put in the effort to do this right. We do limit abbreviation strings to 30 bytes (previously 255), but this seems bigger than the limits used by GNU software, so it should be fine. Still, 30 bytes is a little chunky. But a POSIX time zone is only usable from Jiff's public API via a `TimeZone`, and in that context, it's guarded behind an `Arc`. So we should be fine. (Although, of course, this `Arc` will disappear in core-only mode... Oh well.) Note that we could simply declare that POSIX time zones aren't supported in core-only mode, but they _almost_ don't rely on allocation today. It just requires a little more work. Our short abbreviation strings are now represent by a simple "array string" of fixed capacity. We define a generic array string since it seems plausible that we'll use it again.
It turned out that our feature config testing was somewhat FUBAR.
Namely, we were accidentally and unconditionally always enabling the
`std` feature (among other default features) when running tests. This
was a result of this *dev* dependency:
jiff = { path = "./", features = ["serde"] }
I fixed this by disabling default features to require explicit opt-in:
jiff = { path = "./", default-features = false, features = ["serde"] }
Because this setup languished for a while, there was some accumulated
cruft that warranted some fixing. Mostly this was about gating more code
that isn't used in certain feature configs. I loathe to do this since I
find it abstraction busting, but I'm not sure if there is a great
alternative.
We no longer keep around the "original" string we parsed for POSIX time zones. Instead, we implement `Display` for our POSIX TZ types, and that's how we'll show it in places that need it. This does mean that we show the parsed form instead of what the user actually provided, but I think this should be fine in the vast majority of cases. Notice again how much more annoying this is. A couple of small allocs where it didn't really matter for perf saved us a lot of work. This module does still allocate a `Box<str>` in one place, but it's in code that's only used when `std` is enabled (code for reading the `TZ` environment variable), so we're fine with that for now.
BurntSushi
changed the title
alloc cratealloc crate in src/tz/posix.rs
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is still a fair bit more to do to fully address #168, but this PR gets rid of some allocations
inside of
src/tz/posix.rs. We weren't using them by necessity per se, butit did simplify a fair bit.