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]'d PartialOrd and Ord use variant declaration order, not explicit discriminant values, for (partially) C-like enums #15523

Closed
huonw opened this Issue Jul 8, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@huonw
Copy link
Member

huonw commented Jul 8, 2014

#[deriving(Ord, PartialOrd, Eq, PartialEq, Show)]
enum Foo {
    A = 10,
    B = 0,
    C,
}

fn main() {
    let v = [A, B, C];

    println!("      deriving\tdiscriminant");
    for x in v.iter() {
        for y in v.iter() {
            println!("{}, {}: {}  \t{}", *x, *y, x.cmp(y), (*x as uint).cmp(&(*y as uint)));
        }
    }
}
      deriving  discriminant
A, A: Equal     Equal
A, B: Less      Greater
A, C: Less      Greater
B, A: Greater   Less
B, B: Equal     Equal
B, C: Less      Less
C, A: Greater   Less
C, B: Greater   Greater
C, C: Equal     Equal

That is, deriving considers A the smallest, but it has the largest discriminant.

I would think that #[deriving] should be changed to use the explicit discriminants (if they are available), but there is some possibility that declaration order may be the correct thing (you can always reorder the variants to match their discriminants).

There's also an edge case due to the (apparent?) untypedness of enum variant discriminants: when ordering enum Foo { A = 1, B = -1 }, is the discriminant of B considered negative, or is it positive and huge?

@huonw huonw added the A-syntaxext label Jul 8, 2014

@huonw

This comment has been minimized.

Copy link
Member Author

huonw commented Jul 8, 2014

nominating (for BC-libs, although #[deriving] is on the border between the libraries and language): if we do change this behaviour it would be a breaking change.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jul 10, 2014

Assigning 1.0, P-backcompat-libs.

(I want to write down what the actual ordering that we want for "partial c-like enums" somewhere, as part of solving this.)

@pnkfelix pnkfelix added this to the 1.0 milestone Jul 10, 2014

@pnkfelix pnkfelix removed the I-nominated label Jul 10, 2014

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jul 14, 2014

spawned off of PR #15503

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Jan 8, 2015

Re-nominating. Is this still an issue we want to tackle? If so, it probably belongs on beta.

@aturon aturon added the I-nominated label Jan 8, 2015

@nrc nrc changed the title #[deriving]'d PartialOrd and Ord use variant declaration order, not explicit discriminant values, for (partially) C-like enums #[derive]'d PartialOrd and Ord use variant declaration order, not explicit discriminant values, for (partially) C-like enums Jan 8, 2015

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 8, 2015

leaving on the 1.0 milestone, mostly because if we don't actually get around to doing this, it won't be the end of the world (at least, that's IMO)

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 8, 2015

@huonw will mentor

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Jan 10, 2015

see also #15620 (which strikes me as a logical subtask of this)

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Apr 2, 2015

If we're going to fix this, it needs to be for 1.0.

It seems ilke it would be better to just use the discriminant value.

@pnkfelix pnkfelix self-assigned this Apr 2, 2015

bors added a commit that referenced this issue Apr 10, 2015

Auto merge of #24270 - pnkfelix:use-disr-val-for-derive-ord, r=brson
Use `discriminant_value` intrinsic for `derive(PartialOrd)`

[breaking-change]

This is a [breaking-change] because it can change the result of comparison operators when enum discriminants have been explicitly assigned.  Notably in a case like:
```rust
#[derive(PartialOrd)]
enum E { A = 2, B = 1}
```

Under the old deriving, `A < B` held, because `A` came before `B` in the order of declaration.  But now we use the ordering according to the provided values, and thus `A > B`.  (However, this change is very unlikely to break much, if any, code, since the orderings themselves should all remain well-defined, total, etc.)

Fix #15523

bors added a commit that referenced this issue Apr 10, 2015

Auto merge of #24270 - pnkfelix:use-disr-val-for-derive-ord, r=brson
Use `discriminant_value` intrinsic for `derive(PartialOrd)`

[breaking-change]

This is a [breaking-change] because it can change the result of comparison operators when enum discriminants have been explicitly assigned.  Notably in a case like:
```rust
#[derive(PartialOrd)]
enum E { A = 2, B = 1}
```

Under the old deriving, `A < B` held, because `A` came before `B` in the order of declaration.  But now we use the ordering according to the provided values, and thus `A > B`.  (However, this change is very unlikely to break much, if any, code, since the orderings themselves should all remain well-defined, total, etc.)

Fix #15523

bors added a commit that referenced this issue Apr 10, 2015

Auto merge of #24270 - pnkfelix:use-disr-val-for-derive-ord, r=brson
Use `discriminant_value` intrinsic for `derive(PartialOrd)`

[breaking-change]

This is a [breaking-change] because it can change the result of comparison operators when enum discriminants have been explicitly assigned.  Notably in a case like:
```rust
#[derive(PartialOrd)]
enum E { A = 2, B = 1}
```

Under the old deriving, `A < B` held, because `A` came before `B` in the order of declaration.  But now we use the ordering according to the provided values, and thus `A > B`.  (However, this change is very unlikely to break much, if any, code, since the orderings themselves should all remain well-defined, total, etc.)

Fix #15523

@bors bors closed this in #24270 Apr 11, 2015

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.