Skip to content
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

Tracking issue for #[repr(align(x))] on enums #57996

Closed
niklasf opened this issue Jan 30, 2019 · 9 comments · Fixed by rust-lang/reference#619
Closed

Tracking issue for #[repr(align(x))] on enums #57996

niklasf opened this issue Jan 30, 2019 · 9 comments · Fixed by rust-lang/reference#619
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@niklasf
Copy link
Contributor

niklasf commented Jan 30, 2019

RFC 1358 introduced #[repr(align(x))] on structs (and unions). It mentions:

Custom alignment can only be specified on struct declarations for now. Specifying a different alignment on perhaps enum or type definitions should be a backwards-compatible extension.

It would be a nice convenience to allow this for enums, especially when they are part of a public API.

#[repr(align(x))]
enum Foo {
    // ...
}

would be equivalent to using AlignX<Foo> everywhere:

#[repr(align(x))]
struct AlignX<T>(T);

enum Foo {
    // ...
}
@estebank estebank added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 30, 2019
@Centril Centril added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jan 31, 2019
@Centril Centril changed the title #[repr(align(x))] on enums Tracking issue for #[repr(align(x))] on enums Jan 31, 2019
bors pushed a commit that referenced this issue Feb 6, 2019
bors added a commit that referenced this issue Feb 6, 2019
Allow #[repr(align(x))] on enums (#57996)

Tracking issue: #57996

Implements an extension of [RFC 1358](https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md) behind a feature flag (`repr_align_enum`). Originally introduced here for structs: #39999.

It seems like only HIR-level changes are required, since enums are already aware of their alignment (due to alignment of their limbs).

cc @bitshifter
bors added a commit that referenced this issue Feb 7, 2019
Allow #[repr(align(x))] on enums (#57996)

Tracking issue: #57996

Implements an extension of [RFC 1358](https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md) behind a feature flag (`repr_align_enum`). Originally introduced here for structs: #39999.

It seems like only HIR-level changes are required, since enums are already aware of their alignment (due to alignment of their limbs).

cc @bitshifter
@Centril Centril added the B-unstable Blocker: Implemented in the nightly compiler and unstable. label Feb 11, 2019
@cramertj
Copy link
Member

This compiles today:

#![feature(repr_align_enum)]

#[repr(align(1))]
enum X { X(u64) }

fn main() {
    match &X::X(5) {
        X::X(x) => println!("{:?}", x),
    }
}

I would expect this to be an error-- references to u64 require more than align(1).

@hanna-kruppe
Copy link
Contributor

repr(align(N)) raises the alignment to N, but has no effect if the alignment is already >= N without the annotation (in particular it doesn't lower the alignment or error). Or in other words, it ensures a certain minimum alignment. This is not specific to enums, it works the same for structs and unions. I also think this is intentional, though I can't find mention of it in the RFC at a glance (but in any case I think it's the most sensible semantics -- though perhaps there should be a lint).

@cramertj
Copy link
Member

Got it-- it's surprising to me that we wouldn't at least warn on a #[repr(align(X))] attribute that doesn't have any affect (doesn't raise the alignment).

@Centril
Copy link
Contributor

Centril commented May 18, 2019

@cramertj Seems like something we could tuck into the unused_attributes lint? cc @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2019

The effect of the attribute is platform dependent. I don't see how we could make this lint free of false positives. We could add something like it to clippy, but as a rustc lint that is always on, false positives are essentially never accepted.

Suggesting #[cfg_attr(not(current_platform), repr(align(X)))] is possible, technically correct on all platforms, but still seems like it's just noise to me.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented May 19, 2019

We clearly shouldn't lint for things like #[repr(align(8))] struct Ptr(*const u32) on 64 bit platforms. Deciding where to draw the line (let alone implement that) is difficult to decide in general, but we could approximate it coarsely and conservatively, i.e., err on the side of false negatives. For example, we could try to estimates (edit:) minimum alignment across all plausible target platforms today and only say something if that's possible and the requested alignment is below even that.

@oli-obk
Copy link
Contributor

oli-obk commented May 19, 2019

If that can be done as you describe, adding it to unused_attributes lint sounds great!

@cramertj
Copy link
Member

Okay-- I'm excited about the idea of an unused_attribute lint for this. However, seeing as this is already an issue on stable with #[repr(align(X))] on structs, I don't think we should block stabilization of this feature on the lint, and I think that the rest of @rust-lang/lang agrees. I believe @Centril is planning to write up a stabilization proposal.

@Centril
Copy link
Contributor

Centril commented May 27, 2019

Stabilization PR and proposal up in #61229.
After landing it we will need to slightly adjust the reference before closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants