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

default representation for enum discriminant should be i32 #24290

Closed
pnkfelix opened this Issue Apr 10, 2015 · 6 comments

Comments

Projects
None yet
5 participants
@pnkfelix
Copy link
Member

pnkfelix commented Apr 10, 2015

Spawned off of #24270.

Rust has attempted to support a semantics for enum discriminant representation where, if you did not choose an explicit enum representation, it would infer the appropriate representation to use.

This seems fine as long as one hides that representation from the user, but with intrinsics like discriminant_value, it becomes a bit problematic, since e.g. it is hard to know what type to use when comparing elements of the enum. (You need to cast it to the right type so that you do not e.g. mistake -1 for a large postiive value.)

Plus, it turns out that we don't actually do all that much inference at all; a simple experiment like this:

enum E {
    A = 0,
    B = 0xF_FFFF_FFFF,
}

fn main() { }

when run with a 32-bit target, reveals that the const-evaluator is only using isize for the type anyway, and will not automatically promote to i64.

So, after discussing (irc) the matter with @nikomatsakis I think we change things slightly so that at the language level, in terms of what the const-eval does etc, we should just use i32. (Of course, if you override the choice with #[repr(...)], then we will use that instead.)

This would be a breaking change for 64-bit platforms, but arguably what it is really doing is fixing a portability bug that has laid dormant for at least a little while (and perhaps a long while -- its a long story).

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Apr 10, 2015

triage: P-backcompat-lang (1.0)

@rust-highfive rust-highfive added this to the 1.0 milestone Apr 10, 2015

@pnkfelix pnkfelix self-assigned this Apr 10, 2015

@brson brson removed this from the 1.0 milestone Apr 24, 2015

@brson brson added P-medium and removed P-backcompat-lang labels Apr 24, 2015

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 24, 2015

Not happening for 1.0. Hopefully can be done backwards-compatibly later.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jul 14, 2016

Triage: old issue. Lot's has happened around enums since. @rust-lang/lang what do?

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jul 14, 2016

cc @durka

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jul 14, 2016

@eddyb checked into this; we currently choose the repr based on the value range, so we're closing as resolved.

@aturon aturon closed this Jul 14, 2016

@jonas-schievink

This comment has been minimized.

Copy link
Member

jonas-schievink commented Nov 28, 2018

It... doesn't look like this was ever actually fixed? isize still seems to be the default discriminant type (with no selection according to discriminant values), according to:

enum En {
    V1 = 0xFFFF_FFFF_FFFF_FFFF,
}

which gives this warning on x86_64:

warning: literal out of range for isize
 --> src/lib.rs:2:10
  |
2 |     V1 = 0xFFFF_FFFF_FFFF_FFFF,
  |          ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: #[warn(overflowing_literals)] on by default
  = note: the literal `0xFFFF_FFFF_FFFF_FFFF` (decimal `18446744073709551615`) does not fit into an `isize` and will become `-1isize`
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.