-
Notifications
You must be signed in to change notification settings - Fork 13.4k
use #[align]
attribute for fn_align
#142507
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
3d85cf8
to
8769e96
Compare
This comment has been minimized.
This comment has been minimized.
8769e96
to
92fa35d
Compare
@traviscross the FCP on |
Given the RFC acceptance, this change is good to go as a lang matter. |
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.
Nice, in that case
The Miri subtree was changed cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_attr_data_structures |
On my to-do list! Will take a look somewhere this week :) |
let Some(list) = args.list() else { return }; | ||
|
||
let Some(align) = list.single() else { | ||
cx.dcx() |
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.
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'll leave the scheduling up to you :)
cx: &'c mut AcceptContext<'_, '_, S>, | ||
args: &'c ArgParser<'_>, | ||
) { | ||
// The builtin attributes parser already emits an error in this case. |
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.
by the template logic? I'd quite like to move those checks here at some point, including the templates. For converted attributes I usually have added an exception there to ignore attributes with new-style parsers so that these checks do do something, and all checks happen in one place. If you wouldn't mind, could you do the same for align?
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.
With #138165 you don't need to make a new diagnostic for this either which should make your life easier.
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.
by the template logic?
Exactly
If you wouldn't mind, could you do the same for align?
Do you have an example here? E.g ConfusablesParser
also seems to ignore this case?
fluent::passes_repr_align_function, | ||
) | ||
.emit(); | ||
self.dcx().emit_err(errors::ReprAlignShouldBeAlign { |
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.
love these helpful diagnostics :)
Right now it's used for functions with `fn_align`, in the future it will get more uses (statics, struct fields, etc.)
92fa35d
to
61e20fa
Compare
Tracking issue: #82232
rust-lang/rfcs#3806 decides to add the
#[align]
attribute for alignment of various items. Right now it's used for functions withfn_align
, in the future it will get more uses (statics, struct fields, etc.)(the RFC finishes FCP today)
r? @ghost