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

Misc cleanups #194

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Misc cleanups #194

merged 1 commit into from
Dec 16, 2020

Conversation

bjorn3
Copy link
Collaborator

@bjorn3 bjorn3 commented Dec 16, 2020

Last PR for today. This time I didn't base it on any non-merged PR.

@ltratt
Copy link
Contributor

ltratt commented Dec 16, 2020

Other than "feature gates are undesirable" (which I agree with), is there any motivation for 9146db9? The reason I went with !Sync/!Send is that it's a well-understood way of documenting that a struct isn't send/sync. Although one gets the same type safety from a phantom type, it doesn't quite have the same consistency of documentation. I can be convinced on this, as I tried both designs, and wasn't 100% convinced I chose the best.

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Dec 16, 2020

rust-lang/rust#45198 used PhantomData<*mut Fn()> to prevent all OIBITs. That is stronger than we need here, but I believe PhantomData<...> is generally used over impl !... for ... in the standard library.

@ltratt
Copy link
Contributor

ltratt commented Dec 16, 2020

OK, I can see some instances of this idiom in the standard library (not many, but a few), so I'm semi-swayed. Let's use their precedent and call the attribute either marker or (probably better) _dont_send_or_sync_me. I think we can delete the doc test too. If Rust makes raw pointers Send or Sync, this entire code base will collapse in on itself ;)

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Dec 16, 2020

Renamed and dropped doc comment.

@ltratt
Copy link
Contributor

ltratt commented Dec 16, 2020

Please squash.

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Dec 16, 2020

Splat

@ltratt
Copy link
Contributor

ltratt commented Dec 16, 2020

bors r+

bors bot added a commit that referenced this pull request Dec 16, 2020
194: Misc cleanups r=ltratt a=bjorn3



Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

Build failed:

@bjorn3
Copy link
Collaborator Author

bjorn3 commented Dec 16, 2020

I dropped the first commit. Locally it showed an unused import warning and compiled fine with removing the import, but on CI it is necessary.

@ltratt
Copy link
Contributor

ltratt commented Dec 16, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

Build succeeded:

@bors bors bot merged commit 3d48ab0 into ykjit:master Dec 16, 2020
@bjorn3 bjorn3 deleted the misc_cleanups branch December 16, 2020 22:14
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.

2 participants