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

allocate exact capacity for RFC 9110 strings #198

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

zacknewman
Copy link
Contributor

fmt::rfc2822::DateTimePrinter::timestamp_to_rfc9110_string allocates a String with capacity of 4; however RFC 9110-preferred-format strings are guaranteed to have an exact length of 29. I realize this crate doesn't prioritize performance; however the fact that String::with_capacity was already used as opposed to String::new suggests this isn't an entirely inappropriate diff.

@zacknewman
Copy link
Contributor Author

The CI failure is obviously not related to this PR.

Copy link
Owner

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this looks good!

FWIW, "not prioritizing performance" doesn't mean things shouldn't be optimized or fast or that Jiff doesn't care about being fast. The key word here is prioritization. Prioritization is about balance and what to do when multiple things are in tension with one another. For example, the Zoned and Span types both absolutely give up performance in some cases when compared to more spartan alternatives, but they provide more convenience and make it more difficult to shoot yourself in the foot.

For something like changing the initial capacity, there's really nothing that it is in tension with. And for sure, I have spent a fair bit of time optimizing aspects of Jiff. For example, I spent a lot of effort on an entire benchmark for one aspect of parsing the friendly duration format.

@BurntSushi BurntSushi merged commit c9c3ec5 into BurntSushi:master Jan 12, 2025
17 of 18 checks passed
@zacknewman
Copy link
Contributor Author

zacknewman commented Jan 12, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants