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

bluss · 2015-05-20T14:51:29Z

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 · 2015-05-20T14:51:38Z

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 · 2015-05-20T14:52:30Z

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.

josephglanville · 2015-05-22T17:29:34Z

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 · 2015-05-26T16:35:36Z

@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 · 2015-05-26T16:55:55Z

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 · 2015-05-26T17:34:25Z

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 · 2015-05-26T20:25:28Z

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 · 2015-05-26T22:11:58Z

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

bluss · 2015-05-26T22:26:13Z

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 · 2015-05-27T14:33:39Z

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

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).

bluss · 2015-05-27T18:49:06Z

Oh, my tests are actually completely wrong

bluss · 2015-05-27T19:00:05Z

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

- Successful merges: #25128, #25651, #25668, #25726, #25760, #25792, #25806, #25807 - Failed merges:

alexcrichton · 2015-05-27T22:58:41Z

src/test/run-pass/enum-discrim-manual-sizing-2.rs

+    assert!(size_of::<EC<&i32>>() > ptrsize);
+
+    // test that a zero payload does not influence the discriminant
+    let zero: &i32 = unsafe { zeroed() };


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

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?

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

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

So we should remove this?

Yeah I think it's fine to remove.

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).

bluss · 2015-05-28T18:18:44Z

Thanks, PR updated. Only tests sizes now.

alexcrichton · 2015-05-28T20:36:15Z

@bors: r+ d0e6396

bluss · 2015-05-28T20:44:47Z

Thanks!

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).

bors · 2015-05-29T03:29:43Z

⌛ Testing commit d0e6396 with merge c449a9f...

bors · 2015-05-29T04:16:15Z

💔 Test failed - auto-mac-32-opt

bluss · 2015-05-29T07:12:15Z

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);


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]?

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

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).

bluss · 2015-05-29T19:21:45Z

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

steveklabnik · 2015-05-29T19:24:27Z

@bors: r-

bluss · 2015-06-08T22:30:19Z

Closing for now, reported a bug.

rust-highfive assigned steveklabnik May 20, 2015

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

Manishearth mentioned this pull request May 27, 2015

Rollup of 8 pull requests #25827

Merged

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

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

Auto merge of #25827 - Manishearth:rollup, r=Manishearth

47f9e52

- Successful merges: #25128, #25651, #25668, #25726, #25760, #25792, #25806, #25807 - Failed merges:

alexcrichton reviewed May 27, 2015
View reviewed changes

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

steveklabnik mentioned this pull request May 29, 2015

Rollup of 11 pull requests #25870

Closed

steveklabnik mentioned this pull request May 29, 2015

Rollup of 10 pull requests #25874

Closed

bluss reviewed May 29, 2015
View reviewed changes

steveklabnik mentioned this pull request May 29, 2015

Rollup of 10 pull requests #25887

Closed

bluss mentioned this pull request Jun 8, 2015

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

Closed

bluss closed this Jun 8, 2015

nodakai mentioned this pull request Feb 16, 2016

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

Closed

TimNN mentioned this pull request Jul 15, 2016

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

Closed

bluss mentioned this pull request Feb 22, 2017

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

Closed

bluss mentioned this pull request Oct 23, 2017

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

Merged

bluss deleted the doc-enum-repr branch October 23, 2017 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bluss commented May 20, 2015

rust-highfive commented May 20, 2015

bluss commented May 20, 2015

josephglanville commented May 22, 2015

alexcrichton commented May 26, 2015

bluss commented May 26, 2015

SimonSapin commented May 26, 2015

bluss commented May 26, 2015

SimonSapin commented May 26, 2015

bluss commented May 26, 2015

SimonSapin commented May 27, 2015

bluss commented May 27, 2015

bluss commented May 27, 2015

alexcrichton May 27, 2015

bluss May 28, 2015

alexcrichton May 28, 2015

bluss May 28, 2015

alexcrichton May 28, 2015

bluss commented May 28, 2015

alexcrichton commented May 28, 2015

bluss commented May 28, 2015

bors commented May 29, 2015

bors commented May 29, 2015

bluss May 29, 2015

alexcrichton May 29, 2015

bluss commented May 29, 2015

steveklabnik commented May 29, 2015

bluss commented Jun 8, 2015

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

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

Conversation

bluss commented May 20, 2015

rust-highfive commented May 20, 2015

bluss commented May 20, 2015

josephglanville commented May 22, 2015

alexcrichton commented May 26, 2015

bluss commented May 26, 2015

SimonSapin commented May 26, 2015

bluss commented May 26, 2015

SimonSapin commented May 26, 2015

bluss commented May 26, 2015

SimonSapin commented May 27, 2015

bluss commented May 27, 2015

bluss commented May 27, 2015

alexcrichton May 27, 2015

Choose a reason for hiding this comment

bluss May 28, 2015

Choose a reason for hiding this comment

alexcrichton May 28, 2015

Choose a reason for hiding this comment

bluss May 28, 2015

Choose a reason for hiding this comment

alexcrichton May 28, 2015

Choose a reason for hiding this comment

bluss commented May 28, 2015

alexcrichton commented May 28, 2015

bluss commented May 28, 2015

bors commented May 29, 2015

bors commented May 29, 2015

bluss May 29, 2015

Choose a reason for hiding this comment

alexcrichton May 29, 2015

Choose a reason for hiding this comment

bluss commented May 29, 2015

steveklabnik commented May 29, 2015

bluss commented Jun 8, 2015