-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Store core::str::CharSearcher::utf8_size as u8 #119808
Store core::str::CharSearcher::utf8_size as u8 #119808
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
adddd38
to
71a1558
Compare
library/core/src/str/pattern.rs
Outdated
enum Utf8Size { | ||
// Values are indexes, so `- 1` | ||
One = 0, | ||
Two = 1, | ||
Three = 2, | ||
Four = 3, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If wanted, I can change this to use a rustc_scalar_valid_range
attribute, but it seems safer to use the workaround that everyone else in the ecosystem has to perform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be One = 1
and so on, skipping 0. I don't know if that makes this clearer or less clear.
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
r? libs |
Is there some evidence of downstream (i.e., runtime) benefits from this change? My sense is that the added code complexity isn't a great tradeoff without clear justification - the safety invariant is pretty easy to show being true with or without this. |
No, I simply saw an opportunity where there was an inefficiency and fixed it. If you want me to reduce code complexity, I can simply make this a one line PR to change the int size to u8, but that loses the safety benefit. |
I'm ok landing just the u8, but in general it feels like this kind of optimization isn't worthwhile to me without a clear justification, given that it's not changing to overall size of the type. |
As I have mentioned in other PRs, I'm a massive fan of small incremental improvement. Reducing this int size may allow reducing the type size in the future, or at least allows more optimized codegen. |
When these changes increase code complexity, it's a tradeoff that often goes the way of rejecting them. But changing a obviously bigger-than-necessary int to a smaller one doesn't increase complexity, so that sounds like a good idea. I'm not sure whether it's necessarily the best use of our limited review bandwidth though. |
71a1558
to
e927184
Compare
4893842
to
601f2d1
Compare
Done, after messing up by forgetting to commit then force pushing, this is now just moving to u8 so much less complexity. |
@rustbot review |
@@ -40,6 +40,7 @@ | |||
|
|||
use crate::cmp; | |||
use crate::cmp::Ordering; | |||
use crate::convert::TryInto as _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this is necessary, TryInto should be in the prelude.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like core isn't using edition 2021? It's quite weird, might have just been rust-analyzer weirdness in the Rust codebase though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bors r+ rollup |
…-in-type, r=Mark-Simulacrum Store core::str::CharSearcher::utf8_size as u8 This is already relied on being smaller than u8 due to the `safety invariant: utf8_size must be less than 5`, so this helps LLVM optimize and maybe improve copies due to padding instead of unused bytes.
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#119808 (Store core::str::CharSearcher::utf8_size as u8) - rust-lang#121032 (Continue reporting remaining errors instead of silently dropping them) - rust-lang#121041 (Add `Future` and `IntoFuture` to the 2024 prelude) - rust-lang#121230 (Extend Level API) - rust-lang#121272 (Add diagnostic items for legacy numeric constants) - rust-lang#121275 (add test for panicking attribute macros) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#119808 - GnomedDev:encode-charsearcher-size-in-type, r=Mark-Simulacrum Store core::str::CharSearcher::utf8_size as u8 This is already relied on being smaller than u8 due to the `safety invariant: utf8_size must be less than 5`, so this helps LLVM optimize and maybe improve copies due to padding instead of unused bytes.
This is already relied on being smaller than u8 due to the
safety invariant: utf8_size must be less than 5
, so this helps LLVM optimize and maybe improve copies due to padding instead of unused bytes.