Skip to content

/rust

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

Document #[repr] on non-C-like enums #25651

Closed
wants to merge 1 commit into from

Conversation

Reviewers
No reviews
Assignees

@steveklabnik steveklabnik

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@bluss @rust-highfive @josephglanville @alexcrichton @SimonSapin @bors @steveklabnik
@bluss
Copy link
Contributor

bluss commented May 20, 2015

Document #[repr] on non-C-like enums

rustc accepts the following today:

#[repr(u8)]
enum Flag<T> {
    Dropped,
    Alive(T),
}

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with repr that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).

@rust-highfive rust-highfive assigned steveklabnik May 20, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented May 20, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 20, 2015

This documents current behavior.

Is repr(X) on non-C-like enums is a bug? In that case we need to fix that by rejecting code instead.

@bluss bluss force-pushed the bluss:doc-enum-repr branch from 32391fd to 608e57c May 20, 2015

@josephglanville

This comment has been minimized.

Sign in to view
Copy link

josephglanville commented May 22, 2015

I hope it's not a bug, it would be a breaking change if so. I am using the behaviour and I would assume others are to.

The main reason was that the collections::enum_set::CLike is marked unstable and this seemed to do what I need anyways.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented May 26, 2015

@bluss the #[repr] attribute on data-carrying enums is intentional (e.g. to control the size of the discriminant. It's just quite rare that it's actually used.

@bors: r+ 608e57c rollup

Thanks!
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 26, 2015

Great to have that clear!

@SimonSapin and I discovered that this is quite crucial for arrayvec and smallvec -- see issue servo/rust-smallvec#5
@SimonSapin

This comment has been minimized.

Sign in to view
Copy link
Contributor

SimonSapin commented May 26, 2015

It seems very fragile to relay on on #[repr] to influence something left explicitly undefined… Would it make sense to have a "NonNonZero" that cancels the effects of NonZero? Then arrayvec and smallvec could use something like NonNonZero<ManuallyDrop<[T; 8]>> (rust-lang/rfcs#197)
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 26, 2015

The idea of this PR was to make it non-fragile: The needed guarantee is written into the reference and a test to prove it added to the testsuite.
@SimonSapin

This comment has been minimized.

Sign in to view
Copy link
Contributor

SimonSapin commented May 26, 2015

I made a PR into your PR to add another test for the behavior that arrayvec relies on: bluss#1
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 26, 2015

I never knew you could submit PRs to PRs, that's neat.

arrayvec uses mem::uninitialized but same effect. I don't know, I was satisfied with the tests to ensure the option optimization did not occur by just looking at size_of. it does ensure a discriminant is present, doesn't it?
@SimonSapin

This comment has been minimized.

Sign in to view
Copy link
Contributor

SimonSapin commented May 27, 2015

I think it tests a slightly different case, but feel free to take it or not.

Manishearth added a commit to Manishearth/rust that referenced this pull request May 27, 2015

@Manishearth
Rollup merge of rust-lang#25651 - bluss:doc-enum-repr, r=alexcrichton 
Document #[repr] on non-C-like enums

rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
e3c4875

@Manishearth Manishearth referenced this pull request May 27, 2015

Merged

Rollup of 8 pull requests #25827

@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 27, 2015

Oh, my tests are actually completely wrong

@bluss bluss force-pushed the bluss:doc-enum-repr branch from e57f69f to 42e29fa May 27, 2015

@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 27, 2015

Updated PR. Included @SimonSapin's testcase (thanks!), and fixed mine, which were embarrassingly completely incorrect the first time.

bors added a commit that referenced this pull request May 27, 2015

@bors
Auto merge of #25827 - Manishearth:rollup, r=Manishearth 
- Successful merges: #25128, #25651, #25668, #25726, #25760, #25792, #25806, #25807
- Failed merges:
02453f0

bors added a commit that referenced this pull request May 27, 2015

@bors
Auto merge of #25827 - Manishearth:rollup, r=Manishearth 
- Successful merges: #25128, #25651, #25668, #25726, #25760, #25792, #25806, #25807
- Failed merges:
Loading status checks…
47f9e52
src/test/run-pass/enum-discrim-manual-sizing-2.rs Outdated
assert!(size_of::<EC<&i32>>() > ptrsize);

// test that a zero payload does not influence the discriminant
let zero: &i32 = unsafe { zeroed() };

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton May 27, 2015

Member

Is... this not undefined behavior? We explicitly tell LLVM that all &T references are not null, and this is making a null one?

This comment has been minimized.

Sign in to view
Copy link
@bluss

bluss May 28, 2015

Author Contributor

Right. It should never load the value in the zero variable or in its location inside the enum though, can it be safe that way?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton May 28, 2015

Member

Instead of inspecting the payload, could this just assert that the size of the enum is twice the size of a word?

This comment has been minimized.

Sign in to view
Copy link
@bluss

bluss May 28, 2015

Author Contributor

Well that's basically what the tests above already do. They are less specific maybe.

So we should remove this?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton May 28, 2015

Member

Yeah I think it's fine to remove.
Ulrik Sverdrup
Document #[repr] on non-C-like enums 
rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
Loading status checks…
d0e6396

@bluss bluss force-pushed the bluss:doc-enum-repr branch from 42e29fa to d0e6396 May 28, 2015

@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 28, 2015

Thanks, PR updated. Only tests sizes now.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented May 28, 2015

@bors: r+ d0e6396
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 28, 2015

Thanks!

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 29, 2015

@steveklabnik
Rollup merge of rust-lang#25651 - bluss:doc-enum-repr, r=alexcrichton 
Document #[repr] on non-C-like enums

rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
4dc051d

@steveklabnik steveklabnik referenced this pull request May 29, 2015

Closed

Rollup of 11 pull requests #25870

bors added a commit that referenced this pull request May 29, 2015

@bors
Auto merge of #25870 - steveklabnik:rollup, r=steveklabnik 
- Successful merges: #25651, #25760, #25788, #25830, #25832, #25838, #25854, #25857, #25864, #25865, #25866
- Failed merges:
ed3697c

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 29, 2015

@steveklabnik
Rollup merge of rust-lang#25651 - bluss:doc-enum-repr, r=alexcrichton 
Document #[repr] on non-C-like enums

rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
5a0d962

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 29, 2015

@steveklabnik
Rollup merge of rust-lang#25651 - bluss:doc-enum-repr, r=alexcrichton 
Document #[repr] on non-C-like enums

rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
05ec837

@steveklabnik steveklabnik referenced this pull request May 29, 2015

Closed

Rollup of 10 pull requests #25874

bors added a commit that referenced this pull request May 29, 2015

@bors
Auto merge of #25874 - steveklabnik:rollup, r=steveklabnik 
- Successful merges: #25651, #25788, #25830, #25832, #25838, #25854, #25857, #25864, #25865, #25866
- Failed merges:
d3b78dc
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented May 29, 2015

⌛️ Testing commit d0e6396 with merge c449a9f...

bors added a commit that referenced this pull request May 29, 2015

@bors
Auto merge of #25651 - bluss:doc-enum-repr, r=alexcrichton 
Document #[repr] on non-C-like enums

rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
c449a9f
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented May 29, 2015

💔 Test failed - auto-mac-32-opt
src/test/run-pass/enum-discrim-manual-sizing-2.rs
assert_eq!(size_of::<Eu16<()>>(), 2);
assert_eq!(size_of::<Ei32<()>>(), 4);
assert_eq!(size_of::<Eu32<()>>(), 4);
assert_eq!(size_of::<Ei64<()>>(), 8);

This comment has been minimized.

Sign in to view
Copy link
@bluss

bluss May 29, 2015

Author Contributor

thread '<main>' panicked at 'assertion failed:(left == right)(left:4, right:8)', /Users/rustbuild/src/rust-buildbot/slave/auto-mac-32-opt/build/src/test/run-pass/enum-discrim-manual-sizing-2.rs:89

on 32-bit. Looks like there is a bug in #[repr]?

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton May 29, 2015

Member

Hm yes, that sounds like a bug in #[repr]

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 29, 2015

@steveklabnik
Rollup merge of rust-lang#25651 - bluss:doc-enum-repr, r=alexcrichton 
Document #[repr] on non-C-like enums

rustc accepts the following today:

    #[repr(u8)]
    enum Flag<T> {
        Dropped,
        Alive(T),
    }

and it has a good use (it appears to me): this inhibits the non-nullable
pointer optimization that the regular Option<T> and similar enums allow.

Document this in the reference, and add tests to make sure it compiles.

This means that we guarantee with `repr` that the discriminant will be
present and with that size, but not sure if we want to guarantee
anything more (no guarantee on placement in struct).
e344792

@steveklabnik steveklabnik referenced this pull request May 29, 2015

Closed

Rollup of 10 pull requests #25887

bors added a commit that referenced this pull request May 29, 2015

@bors
Auto merge of #25887 - steveklabnik:rollup, r=steveklabnik 
- Successful merges: #25651, #25788, #25861, #25864, #25865, #25866, #25873, #25876, #25883, #25886
- Failed merges:
8ed0e36
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented May 29, 2015

We need to scrub this PR until the bug is fixed. I'll see what I can do though
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented May 29, 2015

@bors: r-

@bluss bluss referenced this pull request Jun 8, 2015

Closed

repr(i64) picks 32-bit discriminant on 32-bit platform #26114

@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

bluss commented Jun 8, 2015

Closing for now, reported a bug.

@bluss bluss closed this Jun 8, 2015

@nodakai nodakai referenced this pull request Feb 16, 2016

Closed

#[repr(C)] silently accepted on ADT enums #31692

@TimNN TimNN referenced this pull request Jul 15, 2016

Closed

ICE on with #[repr(..)] for single-variant enum #34571

@bluss bluss referenced this pull request Feb 22, 2017

Closed

#[repr(X)] should inhibit enum layout optimization #40029

@bluss bluss referenced this pull request Oct 23, 2017

Merged

Refactor type memory layouts and ABIs, to be more general and easier to optimize. #45225

@bluss bluss deleted the bluss:doc-enum-repr branch Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.