Github Allow specifying alignment for functions by repnop · Pull Request #81234...
source link: https://github.com/rust-lang/rust/pull/81234
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
New issue
Allow specifying alignment for functions #81234
Conversation
Fixes #75072
This allows the user to specify alignment for functions, which can be useful for low level work where functions need to necessarily be aligned to a specific value.
I believe the error cases not covered in the match are caught earlier based on my testing so I had them just return None.
r? @lcnr
(rust-highfive has picked a reviewer for you, use r? to override)
Warning
- These commits modify submodules.
This comment has been hidden.
This seems reasonable to me, to add on an unstable basis. (There needs to be a feature-gate added.)
I also think many codebases will want to set this for all functions using a compiler option (e.g. set in a Cargo profile). But supporting it on individual functions seems reasonable as well.
EDIT: We discussed this in today's lang-team meeting, and this was the general consensus:
- Please add a feature-gate (e.g.
func_align) - When this is merged, we'll need a tracking issue
We discussed this in today's @rust-lang/lang meeting, and we're in favor of adding this to nightly, for experimentation, as soon as there's a feature-gate added.
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:
Concerns:
- feature-gate resolved by #81234 (comment)
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
This comment has been hidden.
This comment has been hidden.
I think I added all the requests, any other changes that need made?
@rfcbot resolve feature-gate
Thank you!
Copy link
rfcbot commented 19 days ago
This is now entering its final comment period, as per the review above.
The latest upstream changes (presumably #82443) made this pull request unmergeable. Please resolve the merge conflicts.
Shouldn't this be going through the RFC process?
Shouldn't this be going through the RFC process?
#[repr(align(x))] for enums did not require a RFC either in #57998, which was introduced as an extension to the repr(align) attribute from RFC 1358. It appears to have gone through a similar process and agreement by the lang team here, except this time rfcbot has been involved ^^
Copy link
rfcbot commented 9 days ago
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
The RFC will be merged soon.
Anything else before this is merged? :)
a few nits, after that r=me
let alignment = match &literal.kind {
ast::LitKind::Int(literal, ast::LitIntType::Unsuffixed) => {
if literal.is_power_of_two() {
// rustc_middle::ty::layout::Align restricts align to <= 2^29
if *literal <= 1 << 29 {
Ok(*literal as u32)
} else {
Err("larger than 2^29")
}
} else {
Err("not a power of two")
}
}
_ => Err("not an unsuffixed integer"),
};
Comment on lines
+2848 to +2862
lcnr 4 days ago
Contributor
can you deduplicate this and move it into a function?
&self.tcx.sess.parse_sess,
sym::fn_align,
hint.span(),
"the attribute `repr(align(...))` on functions is unstable",
lcnr 4 days ago
Contributor
nit: copying the way we write this in
@@ -989,16 +989,36 @@ impl CheckAttrVisitor<'tcx> {
for hint in &hints {
let (article, allowed_targets) = match hint.name_or_empty() {
_ if !matches!(target, Target::Struct | Target::Enum | Target::Union) => {
("a", "struct, enum, or union")
_ if !matches!(
lcnr 4 days ago •
Contributor
this already existed before, but why do we match eagerly here 
this seems unnecessary and slightly worsens diagnostics. I would prefer us to remove that match arm and
put a delay_span_bug in the wildcard arm here.
You don't have to do that in this PR if you want though.
The latest upstream changes (presumably #76570) made this pull request unmergeable. Please resolve the merge conflicts.
None yet
No milestone
Successfully merging this pull request may close these issues.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK