Fix for #62691: use the largest niche across all fields #70411
Conversation
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/librustc/ty/layout.rs
Outdated
| variants[i].iter().enumerate().fold(None, |acc, (j, &field)| { | ||
| let niche = match &field.largest_niche { | ||
| Some(niche) => niche, | ||
| _ => return acc, | ||
| }; | ||
| let ns = niche.available(dl); | ||
| if ns <= niche_size { | ||
| return acc; | ||
| } | ||
| match niche.reserve(self, count) { | ||
| Some(pair) => { | ||
| niche_size = ns; | ||
| Some((j, niche, pair.0, pair.1)) | ||
| } | ||
| None => acc, | ||
| } |
There was a problem hiding this comment.
You don't need this complication, just have .max_by_key(|(_, niche, _)| niche.available(dl)) before .and_then(|(_, niche)| Some((i, niche, niche.reserve(self, count)?))).
There was a problem hiding this comment.
I wanted to do that but that's not exactly the same thing. If it fails for the largest niche, but would have succeeded for a smaller niche, we would end up with None instead of using the smaller niche.
Now, maybe this cannot succeed for a smaller niche and fail for a bigger one so in that case that would be indeed best.
There was a problem hiding this comment.
Now, maybe this cannot succeed for a smaller niche and fail for a bigger one so in that case that would be indeed best.
Yeah, that's why we want bigger niches in general: they succeed strictly more often.
There was a problem hiding this comment.
This is what i've done now.
There was a problem hiding this comment.
But looking at the code of Niche::reserve, it seems it can still fail if valid_range_contains despite being the largest niche while a smaller niche would work. or is that really unlikely?
There was a problem hiding this comment.
The calculations are equivalent, I think I even double-checked them using bruteforce for small bitwidths.
niche.reserve(count) is checking niche.available() >= count but using values it has to compute anyway (i.e. the remaining validity range), instead of actually using available.
src/librustc/ty/layout.rs
Outdated
| if let Some((field_index, niche, (niche_start, niche_scalar))) = variants[i] | ||
| .iter() | ||
| .enumerate() | ||
| .filter_map(|(i, &field)| { | ||
| let niche = field.largest_niche.as_ref()?; | ||
| Some((i, niche, niche.reserve(self, count)?)) | ||
| }) | ||
| .max_by_key(|(_, niche, _)| niche.available(dl)) | ||
| { |
There was a problem hiding this comment.
I really like how readable this is!
src/librustc/ty/layout.rs
Outdated
| let mut niche_size = 0; | ||
| if let Some((field_index, niche, niche_start, niche_scalar)) = | ||
| variants[i].iter().enumerate().fold(None, |acc, (j, &field)| { | ||
| let niche = match &field.largest_niche { | ||
| Some(niche) => niche, | ||
| _ => return acc, | ||
| }; | ||
| let ns = niche.available(dl); | ||
| if ns <= niche_size { | ||
| return acc; | ||
| } | ||
| match niche.reserve(self, count) { | ||
| Some(pair) => { | ||
| niche_size = ns; | ||
| Some((j, niche, pair.0, pair.1)) | ||
| } | ||
| None => acc, | ||
| } |
There was a problem hiding this comment.
Consider sprinkling some comments here btw, and/or extracting functions :)
There was a problem hiding this comment.
Hopefully we can keep something close to the first commit, as it's more functional and way easier to explain as "find the largest niche among the fields" + "make sure it can fit our variant count".
|
r=me after #70411 (comment) is addressed (which should be easier now, with the more functional approach - if you want, you can extract some of that into a variable that you then take apart using e.g. |
|
Updated. |
|
@bors r+ Thanks! |
|
📌 Commit 0b00c20 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - rust-lang#68004 (permit negative impls for non-auto traits) - rust-lang#70385 (Miri nits: comment and var name improvement) - rust-lang#70411 (Fix for rust-lang#62691: use the largest niche across all fields) - rust-lang#70417 (parser: recover on `...` as a pattern, suggesting `..`) - rust-lang#70424 (simplify match stmt) Failed merges: r? @ghost
fixes #62691
(The second commit is a small optimization but it makes the code less pretty and i don't know if it is worth it.)