Misc. rustc_codegen_ssa cleanups 🧹
#136439
Conversation
6313520 to
d39abe9
Compare
|
☔ The latest upstream changes (presumably #136845) made this pull request unmergeable. Please resolve the merge conflicts. |
d39abe9 to
a5dff3c
Compare
|
Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs |
|
This file could use some cleanups, that's for sure. Just wanted to mention that the cleanup around the inline attribute might be short-lived as I'm preparing a pr to remove that code completely. Same for optimize. Not a reason to prevent merging this, but it's good to know :) (See #131229) |
|
looked through the changes themselves too. Couldn't find anything wrong, every rewrite seems to implement the exact same logic as it did before correctly. I'd just say this is a lot of commits for the small number of changes you made. I'd maybe see if you can either make it just one, or split it up for the changes per attribute with a slightly more descriptive name. Especially the 2nd commit right now doesn't really add much. |
a5dff3c to
73d5fe1
Compare
| return Err(errors::LinkRlibError::MissingFormat); | ||
| let mut dep_formats = info.dependency_formats.iter(); | ||
| let (ty1, list1) = dep_formats.next().ok_or(errors::LinkRlibError::MissingFormat)?; | ||
| if let Some((ty2, list2)) = dep_formats.find(|(_, list2)| list1 != *list2) { |
There was a problem hiding this comment.
I don't think this is equivalent, because if say the crate type 2 and 3 disagree it will no longer emit an error.
There was a problem hiding this comment.
Is the == operator not transitive for this type?
There was a problem hiding this comment.
It is. But say the dependency formats have 3 elements (where each element is a list), where the the second list and the last list are different. Previously, this was detected, because all combinations are checked, but now it is no longer detected because you only compare the first list against all other lists. The second and third lists are no longer compared.
There was a problem hiding this comment.
I do agree with @kadiwa4 on this. In your example if the second and last lists are different, then the first and last list will also be different, and it would still be caught.
But I might just be completely wrong on this, I dunno 😅
There was a problem hiding this comment.
(assuming the second and first lists don't compare as equal)
There was a problem hiding this comment.
Yeah, that's what I thought.
This is the (slightly shortened) definition of the transitivity property from the std docs:
a == bandb == cimpliesa == c
Now say a = dep_formats[1].1, b = dep_formats[0].1 and c = dep_formats[2].1. In the .find() method, the code checks that b == a and b == c. Assuming that find finishes without finding a mismatch, we now know that according to the transitivity property, a == c is true (i.e. dep_formats[1].1 == dep_formats[2].1).
(I am assuming that b == a is the same as a == b of course.)
Just a bunch of stuff I found while reading the crate's code.
Each commit can stand on its own.
Maybe r? @Noratrieb because I saw you did some similar cleanups on these files a while ago? (feel free to re-assign, I'm just guessing)