[RFC accepted] add wrapper for discriminant_value intrinsic #34785

durka · 2016-07-12T17:19:32Z

add wrapper for discriminant_value intrinsic

Wraps the discriminant_value intrinsic under the name std::mem::discriminant. In order to avoid prematurely leaking information about the implementation of enums, the return value is an opaque type, generic over the enum type, which implements Copy, Clone, PartialEq, Eq, Hash, and Debug (notably not PartialOrd). There is currently no way to get the value out excepting printing the debug representation.

The wrapper is safe and can be stabilized soon as per discussion in #24263.

r? @brson

durka · 2016-07-12T17:26:35Z

I just realized the wrapper should be safe. Another commit incoming...

sfackler · 2016-07-12T17:42:36Z

src/libcore/raw.rs

 pub struct TraitObject {
    pub data: *mut (),
    pub vtable: *mut (),
 }
+
+/// Returns the value of the discriminant for the enum variant in `v`.


It is probably worth adding a warning that discriminants may not compare in the correct order if the enum is #[repr(i64)] due to overflow.

Will do.

On Tue, Jul 12, 2016 at 1:43 PM, Steven Fackler notifications@github.com
wrote:

In src/libcore/raw.rs
#34785 (comment):

pub struct TraitObject {
pub data: *mut (),
pub vtable: *mut (),
}
+
+/// Returns the value of the discriminant for the enum variant in v.

It is probably worth adding a warning that discriminants may not compare
in the correct order if the enum is #[repr(u64)] due to overflow.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/34785/files/609c17e8d0ceac437261df2c6b4231225f1faf08#r70485187,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n4IaNbDuCLcSiYjaKILI3yho7yvLks5qU9IrgaJpZM4JKnnt
.

Wait, I don't understand. You mean if the enum is not #[repr(u64)]?

On Tue, Jul 12, 2016 at 1:46 PM, Alex Burka durka42@gmail.com wrote:

Will do.

On Tue, Jul 12, 2016 at 1:43 PM, Steven Fackler notifications@github.com
wrote:

In src/libcore/raw.rs
#34785 (comment):

pub struct TraitObject {
pub data: *mut (),
pub vtable: *mut (),
}
+
+/// Returns the value of the discriminant for the enum variant in v.

It is probably worth adding a warning that discriminants may not compare
in the correct order if the enum is #[repr(u64)] due to overflow.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/34785/files/609c17e8d0ceac437261df2c6b4231225f1faf08#r70485187,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n4IaNbDuCLcSiYjaKILI3yho7yvLks5qU9IrgaJpZM4JKnnt
.

Er, yeah. Your doc change is what I was thinking of.

alexcrichton · 2016-07-12T17:56:25Z

I might personally prefer this being located in the std::mem module as opposed to std::raw. We've almost deleted the entire std::raw module and I'd personally like to see it go out all the way!

Also, could the actual stabilization be deferred to a later date? The libs team tends to not apply stabilizations mid-cycle but instead places issues into final comment period at the end of which we decide on the stabilization outcome (as a result of the discussion, if any, that happened).

cc @rust-lang/libs, any objections on moving this to FCP though? Seems good to me!

durka · 2016-07-12T17:59:29Z

OK, I had previously asked at the tracking issue if this should be insta-stable (since it is basically stabilizing the intrinsic, even though it doesn't actually do that) and @brson said yes. But I'm happy to mark it unstable for now. I'll move it to std::mem as well (though it doesn't have much if anything to do with memory).

durka · 2016-07-12T18:16:27Z

@sfackler's comment was hidden, but I added a note about comparisons.

alexcrichton · 2016-07-12T18:18:41Z

src/libcore/mem.rs

+/// enum Foo { A(String), B(i32) }
+///
+/// # fn main() {
+/// assert!(mem::discriminant(Foo::A("bar")) != mem::discriminant(Foo::A("baz")));


I think these examples may not compile, the discriminants should be the same here, right? Additionally, shouldn't a shared reference be passed in?

Yeah, I just noticed that and fixed it.

On Tue, Jul 12, 2016 at 2:19 PM, Alex Crichton notifications@github.com
wrote:

In src/libcore/mem.rs
#34785 (comment):

+/// same order as actual enum variants would under #[derive(PartialOrd)].
+///
+/// If T is not an enum, the return value is unspecified.
+///
+/// # Example
+///
+/// This can be used to compare enums that carry data, while disregarding
+/// the actual data:
+///
+/// ```
+/// use std::mem;
+///
+/// enum Foo { A(String), B(i32) }
+///
+/// # fn main() {
+/// assert!(mem::discriminant(Foo::A("bar")) != mem::discriminant(Foo::A("baz")));

I think these examples may not compile, the discriminants should be the
same here, right? Additionally, shouldn't a shared reference be passed in?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/34785/files/3a88d8811ae23af836e8ed4349e4a96656303878#r70492383,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n1nIqX5ydVTZ2ue3rNp94IPEA-khks5qU9qggaJpZM4JKnnt
.

sfackler · 2016-07-12T18:47:36Z

@alexcrichton I want to stabilize type_name or something like it, so we might want to keep std::raw around for things like that.

durka · 2016-07-12T18:50:01Z

Is it the case that a discriminant can never be larger than u64? (I think there is an RFC to add u128 and whatnot...)

durka · 2016-07-12T18:51:12Z

For the record (on the theme of my previous comment) I still think it would be better to use the type system to make this function's return type better. But I also understand we want to stabilize something.

alexcrichton · 2016-07-12T19:52:15Z

@sfackler hm that's a good point about type_name, but I figure we can probably figure out a better way to do that than putting it in std::raw

aturon · 2016-07-13T03:53:32Z

I wasn't initially a fan of std::mem for this, but I can sort of see it now. E.g. this is not so different from things like size_of, which are type-generic functions giving repr details. I could see type_name living here eventually, too.

tbu- · 2016-07-13T10:02:05Z

src/test/run-pass/discriminant_value-wrapper.rs

+    assert_eq!(mem::discriminant(&NullablePointer::Something(&CONST)), 0);
+
+    // unlike the test for the intrinsic, there are no tests with non-enums here
+    // because the result is documented to be unspecified


You could still call it on a struct, just to check that it doesn't panic.

durka · 2016-08-01T18:37:21Z

RFC posted at rust-lang/rfcs#1696.

bors · 2016-08-26T09:23:58Z

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

durka · 2016-08-26T18:55:08Z

Waiting to rebase until the RFC process concludes.

durka · 2016-09-16T03:04:37Z

Rebased and updated.

I removed the Reflect bound as per the RFC (nobody opined on it during FCP). I'm assuming that trait's FCP will end in deprecation; if not, we could revisit.

I updated the stability section of the documentation to say (again, nobody opined during FCP):

Discriminants can change if enum variants are reordered, if a new variant is addedin the middle, or (in the case of a C-like enum) if explicitly set discriminants are changed. Therefore, relying on the discriminants of enums outside of your crate may be a poor decision. However, discriminants of an identical enum should not change between subsequent compilations with the same compiler.

aturon · 2016-09-27T19:34:37Z

@nagisa Can you take over this review, given that you've already looked at the PR?

nagisa · 2016-09-27T19:38:45Z

src/libcore/mem.rs

+///
+/// # Stability
+///
+/// Discriminants can change if enum variants are reordered, if a new variant is added


As the return value of this wrapper is a opaque value Discriminant<T>, this section reads weird. Namely it changing makes little sense because you aren’t supposed to be able to inspect it directly. Rather the stability and behaviour should be described in terms of PartialEq and Hash.

That's a good point.

nagisa · 2016-09-27T19:39:31Z

src/libcore/mem.rs

+#[unstable(feature = "discriminant_value", reason = "recently added, follows RFC", issue = "24263")]
+impl<T> fmt::Debug for Discriminant<T> {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
+        self.0.fmt(fmt)


This probably wants to use a debug builder and output something along the lines of Discriminant(<number>), rather than outputting a plain number.

nagisa · 2016-09-27T19:51:15Z

src/libcore/mem.rs

+pub struct Discriminant<T>(u64, PhantomData<*const T>);
+
+#[unstable(feature = "discriminant_value", reason = "recently added, follows RFC", issue = "24263")]
+impl<T> Copy for Discriminant<T> {}


Why not derive all these implementations?

You'd get nonsense bounds on T.

I thought I'd included a comment to this effect. I'll add one.

durka · 2016-09-27T23:31:17Z

I think it's ready, just waiting for tests.

aturon · 2016-09-27T23:49:08Z

@durka OK, just give a ping once it's ready for bors and i'll send it along.

durka · 2016-09-28T00:23:11Z

Tests and tidy pass locally.

aturon · 2016-09-28T03:13:45Z

Huzzah!

@bors: r=nagisa

durka · 2016-09-28T03:50:45Z

@bors? you there?

aturon · 2016-09-28T04:06:55Z

@bors: r+

aturon · 2016-09-28T04:43:12Z

@bors r=nagisa

aturon · 2016-09-28T04:43:53Z

@bors: r=nagisa

durka · 2016-09-29T06:00:13Z

Should I try reopening as a new PR?

durka · 2016-09-29T07:17:16Z

Moved to #36823.

@bors

add wrapper for discriminant_value, take 2 [This is #34785 reopened, since @bors apparently gave up on that thread.] add wrapper for discriminant_value intrinsic Implementation of [RFC 1696](https://github.com/rust-lang/rfcs/blob/master/text/1696-discriminant.md). Wraps the `discriminant_value` intrinsic under the name `std::mem::discriminant`. In order to avoid prematurely leaking information about the implementation of enums, the return value is an opaque type, generic over the enum type, which implements Copy, Clone, PartialEq, Eq, Hash, and Debug (notably not PartialOrd). There is currently no way to get the value out excepting printing the debug representation. The wrapper is safe and can be stabilized soon as per discussion in #24263. cc @aturon r? @nagisa

rust-highfive assigned brson Jul 12, 2016

This was referenced Jul 12, 2016

discriminant_value intrinsic -- tracking issue for 639 #24263

Closed

Tracking issue for raw stabilization (raw::TraitObject) #27751

Closed

durka force-pushed the discriminant_value branch 3 times, most recently from 38a41cb to 609c17e Compare July 12, 2016 17:24

sfackler reviewed Jul 12, 2016
View reviewed changes

durka force-pushed the discriminant_value branch 2 times, most recently from 29e3b0d to 1001205 Compare July 12, 2016 18:16

durka force-pushed the discriminant_value branch 2 times, most recently from eb26969 to 3a88d88 Compare July 12, 2016 18:17

alexcrichton reviewed Jul 12, 2016
View reviewed changes

durka force-pushed the discriminant_value branch 3 times, most recently from 987979d to c35ec62 Compare July 12, 2016 18:26

durka changed the title ~~add stable wrapper for discriminant_value intrinsic~~ add soon-to-be-stable wrapper for discriminant_value intrinsic Jul 12, 2016

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 12, 2016

durka force-pushed the discriminant_value branch from c35ec62 to 90e4530 Compare July 13, 2016 02:27

tbu- reviewed Jul 13, 2016
View reviewed changes

durka mentioned this pull request Aug 1, 2016

mem::discriminant() rust-lang/rfcs#1696

Merged

nikomatsakis removed the I-nominated label Aug 4, 2016

durka changed the title ~~[WIP] add wrapper for discriminant_value intrinsic~~ [RFC accepted] add wrapper for discriminant_value intrinsic Sep 16, 2016

durka force-pushed the discriminant_value branch from a7de3ad to a8c3cf6 Compare September 16, 2016 03:01

oli-obk mentioned this pull request Sep 19, 2016

Add as_borrow() generic method for Value serde-rs/json#153

Closed

nagisa reviewed Sep 27, 2016

View reviewed changes

durka force-pushed the discriminant_value branch from a8c3cf6 to e447d42 Compare September 27, 2016 22:06

add wrapper for discriminant_value

a84b550

durka force-pushed the discriminant_value branch from e447d42 to a84b550 Compare September 27, 2016 23:29

aturon closed this Sep 28, 2016

aturon reopened this Sep 28, 2016

durka closed this Sep 29, 2016

durka mentioned this pull request Sep 29, 2016

add wrapper for discriminant_value, take 2 #36823

Merged

[RFC accepted] add wrapper for discriminant_value intrinsic #34785

[RFC accepted] add wrapper for discriminant_value intrinsic #34785

Conversation

durka commented Jul 12, 2016 • edited Loading

durka commented Jul 12, 2016

sfackler Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 12, 2016

durka commented Jul 12, 2016 • edited Loading

durka commented Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Jul 12, 2016

durka commented Jul 12, 2016 • edited Loading

durka commented Jul 12, 2016

alexcrichton commented Jul 12, 2016

aturon commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durka commented Aug 1, 2016

bors commented Aug 26, 2016

durka commented Aug 26, 2016

durka commented Sep 16, 2016

aturon commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durka commented Sep 27, 2016

aturon commented Sep 27, 2016

durka commented Sep 28, 2016

aturon commented Sep 28, 2016

durka commented Sep 28, 2016

aturon commented Sep 28, 2016

aturon commented Sep 28, 2016

aturon commented Sep 28, 2016

durka commented Sep 29, 2016

durka commented Sep 29, 2016

durka commented Jul 12, 2016 •

edited

Loading

sfackler Jul 12, 2016 •

edited

Loading

durka commented Jul 12, 2016 •

edited

Loading

durka commented Jul 12, 2016 •

edited

Loading

durka commented Jul 12, 2016 •

edited

Loading

nagisa Sep 27, 2016 •

edited

Loading