Skip to content

add wrapper for discriminant_value, take 2 #36823

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

Merged
merged 1 commit into from
Sep 30, 2016

Conversation

durka
Copy link
Contributor

@durka durka commented Sep 29, 2016

[This is #34785 reopened, since @bors apparently gave up on that thread.]

add wrapper for discriminant_value intrinsic

Implementation of RFC 1696.

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

assert!(mem::discriminant(&ADT::Second(5)) == mem::discriminant(&ADT::Second(6)));
assert!(mem::discriminant(&ADT::First(2,2)) != mem::discriminant(&ADT::Second(2)));

let _ = mem::discriminant(&10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be `assert_eq!(mem::discriminant(&10), mem::discriminant(&20));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value for non-enums is unspecified but that doesn't imply it's the same for all non-enums (it is zero, in truth, but...).

@nagisa
Copy link
Member

nagisa commented Sep 29, 2016

It is undefined value. No purpose in testing it.

On Sep 29, 2016 3:25 PM, "Oliver Schneider" [email protected]
wrote:

@oli-obk commented on this pull request.

In src/test/run-pass/discriminant_value-wrapper.rs
#36823 (review):

+#![feature(discriminant_value)]
+
+use std::mem;
+
+enum ADT {

  • First(u32, u32),
  • Second(u64)
    +}

+pub fn main() {

  • assert!(mem::discriminant(&ADT::First(0,0)) == mem::discriminant(&ADT::First(1,1)));
  • assert!(mem::discriminant(&ADT::Second(5)) == mem::discriminant(&ADT::Second(6)));
  • assert!(mem::discriminant(&ADT::First(2,2)) != mem::discriminant(&ADT::Second(2)));
  • let _ = mem::discriminant(&10);

this could be `assert_eq!(mem::discriminant(&10), mem::discriminant(&20));


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#36823 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AApc0qecoWekxtQqzeQjSOujUb4_4uDLks5qu65OgaJpZM4KJp4d
.


/// Opaque type representing the discriminant of an enum.
///
/// See the `discriminant` function in this module for more information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could link to the function in question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can do this in a follow-up PR since this already got r+'ed.

@sfackler
Copy link
Member

@bors r=nagisa

@bors
Copy link
Collaborator

bors commented Sep 29, 2016

📌 Commit a84b550 has been approved by nagisa

@bors
Copy link
Collaborator

bors commented Sep 30, 2016

⌛ Testing commit a84b550 with merge 9548730...

bors added a commit that referenced this pull request Sep 30, 2016
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
/// Opaque type representing the discriminant of an enum.
///
/// See the `discriminant` function in this module for more information.
#[unstable(feature = "discriminant_value", reason = "recently added, follows RFC", issue = "24263")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the "reason" field is no longer required since the contents always ended up saying basically the same thing.

@durka
Copy link
Contributor Author

durka commented Sep 30, 2016

Good to know.

On Fri, Sep 30, 2016 at 11:24 AM, Steven Fackler [email protected]
wrote:

@sfackler commented on this pull request.

In src/libcore/mem.rs
#36823 (review):

@@ -647,3 +652,80 @@ pub fn drop(_x: T) { }
pub unsafe fn transmute_copy<T, U>(src: &T) -> U {
ptr::read(src as *const T as *const U)
}
+
+/// Opaque type representing the discriminant of an enum.
+///
+/// See the discriminant function in this module for more information.
+#[unstable(feature = "discriminant_value", reason = "recently added, follows RFC", issue = "24263")]

IIRC the "reason" field is no longer required since the contents always
ended up saying basically the same thing.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#36823 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3nxHT-G7CiSr9D4Logh65smtxHdc2ks5qvSmegaJpZM4KJp4d
.

@bors bors merged commit a84b550 into rust-lang:master Sep 30, 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.

6 participants