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

derive: assume enum repr defaults to isize #32253

Merged
merged 2 commits into from
Mar 22, 2016
Merged

Conversation

durka
Copy link
Contributor

@durka durka commented Mar 14, 2016

derive: assume enum repr defaults to isize

Fixes #31886.

Spawned from #32139.

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+ e45b59aed463c15bd9aadd8280f4a5d49027e73f

@bors
Copy link
Contributor

bors commented Mar 14, 2016

☔ The latest upstream changes (presumably #30587) made this pull request unmergeable. Please resolve the merge conflicts.

@durka
Copy link
Contributor Author

durka commented Mar 14, 2016

@oli-obk is this fix now irrelevant given your PR?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2016

well kindof.

I actually broke the following construct (explicitly, not accidentally) on 32bit

 enum Eu64 {
     Au64 = 0,
     Bu64 = 0x8000_0000_0000_0000
 }

because on 32bit rustc now errors at compiletime about the collision

Once we properly fix the automatic enum discriminant type detection, the issue might appear again.

@durka
Copy link
Contributor Author

durka commented Mar 15, 2016

@oli-obk can you clarify what it means to properly fix discriminant detection, and what the current situation is? Is deriving correct in assuming that if the repr isn't specified than it will be isize (or smaller)?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 15, 2016

@durka

In list form, because I'm confusing myself when I'm writing it in text form.

  1. -9223372036854775808isize as u64 always was 0 on 32bit and still is.
  2. Eu64::Bu64 as u64 always was 0 on 32bit and still is
    1. makes zero sense, as the enum variant discriminants can't both be zero
  3. size_of::<Eu64>() == 8 on 32bit and 64bit
  4. I broke the mentioned enum, because my typestrong const int branch ends up turning the discriminant of Eu64::Bu64 to 0 early enough to allow rustc to detect the collision.
  5. Now there's no way to specify a single value that ends up turning the discriminant to be 8 bytes. (the Ei64 example uses two values, a negative one and a large positive one)
  6. I'm not sure how to reproduce very slow derived PartialEq/PartialOrd implementation of enum #31886 now, but Ei64 is still 8 bytes, while the discriminant type is isize.

can you clarify what it means to properly fix discriminant detection

well... the discriminant should be i64 and u64 imo, it should never assume isize/usize automatically, because that's really not portable at all. If someone wants pointer-size, then they should annotate it with repr. But that would technically be a breaking change for anyone ignoring the literal overflow warnings.

@durka
Copy link
Contributor Author

durka commented Mar 17, 2016

@oli-obk the code given in #31886 still builds (and hangs) on nightly-2016-03-16.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 18, 2016

oh.... I only tested on 32 bit... it obviously only collides on 32 bit... So yea, this change is still necessary for 64 bit.

durka added 2 commits March 18, 2016 15:03
It was originally intended to be i32, but it isn't.

Fixes rust-lang#31886.
@durka durka changed the title derive: assume enum repr defaults to i64 derive: assume enum repr defaults to isize Mar 18, 2016
@durka
Copy link
Contributor Author

durka commented Mar 18, 2016

Okay, so I changed it back to isize since now @oli-obk's changes should prevent an enum with colliding variants from compiling.

Note that you can still write a program like this:

enum Eu64 {
    Au64 = 0,
    Bu64 = 0x8000_0000_0000_0001 //~WARN literal out of range for isize
}

fn main() {
    println!("{} {} {}", std::mem::size_of::<Eu64>(), Eu64::Au64 as isize, Eu64::Bu64 as isize);
}

which prints different answers on 32- and 64-bit (1 0 1 and 8 0 -9223372036854775807 respectively).

@durka
Copy link
Contributor Author

durka commented Mar 18, 2016

And combined with the fact that #[derive(PartialEq, PartialOrd)] uses discriminant_value, the above enum will give different answers for Au64 < Bu64...

I guess that's the price you pay for ignoring the warnings.

@alexcrichton
Copy link
Member

Sorry I've gotten a bit lost with all the changes in flight. Is this ready to go? Did something about constants change how we want to do this?

@durka
Copy link
Contributor Author

durka commented Mar 21, 2016

@alexcrichton what I believe is that is no longer possible to trigger #31886 on a 32-bit system because @oli-obk's const evaluator will catch the collision. However, my fix (as implemented currently, in this PR!) is still needed to avoid #31886 on 64-bit systems.

@alexcrichton
Copy link
Member

@bors: r+ 9d0748f

Ok, thanks for the explanation @durka!

@bors
Copy link
Contributor

bors commented Mar 21, 2016

⌛ Testing commit 9d0748f with merge 2ddcf31...

@bors
Copy link
Contributor

bors commented Mar 21, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@oli-obk
Copy link
Contributor

oli-obk commented Mar 21, 2016

spurious failure

Makefile:26: recipe for target 'clean' failed
Zombie process: "\Device\HarddiskVolume1\dojob.exe"
thread '

' panicked at 'fs::remove_dir_all(path) failed with Access is denied. (os error 5)', C:\bot\slave\auto-win-msvc-64-opt-rustbuild\build\src\bootstrap\build\clean.rs:34
note: Run with RUST_BACKTRACE=1 for a backtrace.
make: *** [clean] Error 101

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Mar 22, 2016

⌛ Testing commit 9d0748f with merge 6cc502c...

bors added a commit that referenced this pull request Mar 22, 2016
derive: assume enum repr defaults to isize

derive: assume enum repr defaults to isize

Fixes #31886.

Spawned from #32139.

r? @alexcrichton
@bors bors merged commit 9d0748f into rust-lang:master Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants