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

Replace i128_support feature with compile-time detection #571

Merged
merged 2 commits into from
Jul 30, 2018

Conversation

vks
Copy link
Collaborator

@vks vks commented Jul 27, 2018

This breaks builds using the i128_support feature with nightlies
supporting i128 before Rust 1.26. I think this is acceptable, unless we want to support very old nightlies.

@vks vks changed the title Replacde i128_support feature with compile-time detection Replace i128_support feature with compile-time detection Jul 27, 2018
@dhardy
Copy link
Member

dhardy commented Jul 27, 2018

I argued against this before (#459) and I'm still not sure it's a good idea. The theoretical disadvantage is that projects may not support older compilers even though they trivially could.

In fact, this implementation is worse, in that it completely breaks builds with old nightlies using i128. This isn't necessary.

The status quo is that users needing i128_support must pass the feature flag, and this then works on current compilers, older compilers (only nightlies before 1.26), and in the future — at some point the flag will become redundant, and maybe eventually removed.

@vks
Copy link
Collaborator Author

vks commented Jul 27, 2018

The theoretical disadvantage is that projects may not support older compilers even though they trivially could.

As far as I can tell, this would only apply to nightlies older than 1.26.

In fact, this implementation is worse, in that it completely breaks builds with old nightlies using i128. This isn't necessary.

It's true that it's not necessary to break those nightlies, but do we really want to commit to supporting old nightlies? Note that in general this is not possible, because supporting new nightlies often means breaking old nightlies. Due to this, I think it is fair to assume that nightlies are at least as young as the latest stable release. (We should document this though.)

at some point the flag will become redundant, and maybe eventually removed.

It will only become redundant when we enable it by default, resulting in much worse breakage than this PR causes.

@dhardy dhardy added the E-question Participation: opinions wanted label Jul 27, 2018
@dhardy dhardy mentioned this pull request Jul 27, 2018
@dhardy
Copy link
Member

dhardy commented Jul 28, 2018

I guess you're right; trying to support old nightlies is ridiculous. Maybe we should never even have allowed the i128_support feature flag on stable (in fact, with auto-detection, it needn't ever have been introduced).

Okay, then this has my approval. We could add a note in the README. We should in the CHANGELOG. I guess it makes sense to leave the dummy feature flag until later and remove in the 0.7 release.

Any further comments? Otherwise I'll merge in 1-2 days.

@vks
Copy link
Collaborator Author

vks commented Jul 30, 2018

Should we add "Nightlies older than the last stable version of Rust are not supported." to the README?

@dhardy
Copy link
Member

dhardy commented Jul 30, 2018

But we can't even guarantee support for nightlies newer than the last stable version... so I don't think that's necessary.

Just a note in the changelog here then and we're done?

vks added 2 commits July 30, 2018 13:33
This breaks builds using the `i128_support` feature with nightlies
supporting `i128` before Rust 1.26.
@vks
Copy link
Collaborator Author

vks commented Jul 30, 2018

I updated the changelog (for a few other changes as well) and rebased on master.

@dhardy dhardy merged commit 2f052a9 into rust-random:master Jul 30, 2018
@vks vks deleted the automatic-i128-support branch July 30, 2018 12:06
@hcpl hcpl mentioned this pull request Nov 12, 2018
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-question Participation: opinions wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants