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

Ensure std::mem::Discriminant is Send + Sync #45095

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

bluss
Copy link
Member

@bluss bluss commented Oct 7, 2017

PhantomData<*const T> has the implication of Send / Syncness following
the *const T type, but the discriminant should always be Send and Sync.

Use PhantomData<fn() -> T> which has the same variance in T, but is Send + Sync

@rust-highfive
Copy link
Collaborator

r? @BurntSushi

(rust_highfive has picked a reviewer for you, use r? to override)

@bluss
Copy link
Member Author

bluss commented Oct 7, 2017

Tracking issue #24263

@bluss
Copy link
Member Author

bluss commented Oct 7, 2017

As discussed before on irc, a bit unclear what the correct phantom data really is for this thing.

`PhantomData<*const T>` has the implication of Send / Syncness following
the *const T type, but the discriminant should always be Send and Sync.

Use `PhantomData<fn() -> T>` which has the same variance in T, but is Send + Sync
@BurntSushi
Copy link
Member

I am throwing this to @aturon. I don't immediately see anything wrong with this, but I'd like someone else to see it! cc @rust-lang/libs

@BurntSushi BurntSushi assigned aturon and unassigned BurntSushi Oct 8, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

What are the tradeoffs of using a screwy PhantomData marker vs unsafe impl Send?

@ghost
Copy link

ghost commented Oct 8, 2017

Sometimes I wish we had a few additional marker structs so we could do:

_marker: PhantomData<Variant<T>>
_marker: PhantomData<Invariant<T>>
_marker: PhantomData<(T, NotSendSync)>

@alexcrichton
Copy link
Member

Looks good to me!

@dtolnay I think the usage of fn() -> T helps avoid the usage of unsafe impl, which seems like a bonus to me!

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2017
@carols10cents
Copy link
Member

Just a note that @aturon is on PTO this week so isn't likely to get to it, but it looks like this one is worth waiting for him to take a look.

@alexcrichton
Copy link
Member

I'm gonna go ahead and approve this as I think there won't be too much opposition, and we'll have a long time on nightly before this reaches stable as well!

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 9, 2017
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 9, 2017

📌 Commit 3fff2d9 has been approved by alexcrichton

kennytm added a commit to kennytm/rust that referenced this pull request Oct 10, 2017
…xcrichton

Ensure std::mem::Discriminant is Send + Sync

`PhantomData<*const T>` has the implication of Send / Syncness following
the *const T type, but the discriminant should always be Send and Sync.

Use `PhantomData<fn() -> T>` which has the same variance in T, but is Send + Sync
bors added a commit that referenced this pull request Oct 10, 2017
Rollup of 9 pull requests

- Successful merges: #44775, #45089, #45095, #45099, #45101, #45108, #45116, #45135, #45146
- Failed merges:
@bors bors merged commit 3fff2d9 into rust-lang:master Oct 10, 2017
@bluss bluss deleted the discriminant-send-sync branch October 29, 2017 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants