Skip to content

Use crate visibility for traits #358

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
May 26, 2020
Merged

Use crate visibility for traits #358

merged 1 commit into from
May 26, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented May 26, 2020

Those traits are an implementation detail of compiler builtins and not intended
for general use. The fact that they are public not only makes it possible to
use them, but also leads to situation where rustc is providing them as a suggestion:

@alexcrichton
Copy link
Member

Is there a particular reason for this? Could you explain in the commit or PR message as to why this change is being made?

@bjorn3
Copy link
Member

bjorn3 commented May 26, 2020

The fact that these traits are exported has been useful to me while debugging miscompilations by cg_clif around 128bit int handling. Because these traits are exported, I can just import them after copying an intrinsic provided by compiler-builtins to the test crate. I don't have to copy all the methods provided by these traits too until I actually want to change them.

@tmiasko
Copy link
Contributor Author

tmiasko commented May 26, 2020

I added rationale to the pull request description.

@alexcrichton
Copy link
Member

Ok thanks! This sounds like a reasonable stopgap to me for now until a better solution is worked out. @bjorn3 I think that's not really the intended use of this crate so while it may be helpful I think it's more important to cater to rust-lang/rust's issues it's having right now.

@alexcrichton alexcrichton merged commit d0ea076 into rust-lang:master May 26, 2020
@tmiasko tmiasko deleted the crate-traits branch May 26, 2020 21:11
@jethrogb
Copy link
Contributor

I think maybe this should've led to a minor version bump due to the public API change?

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.

4 participants