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

Allow some deprecated items #2117

Closed
wants to merge 2 commits into from

Conversation

killercup
Copy link
Member

These were deprecated recently but we don't want to use them yet to not
break compatibility with older rustc versions unnecessary. Explicitly
allowing them should fix the build for nightly with #[deny(warnings)].

@killercup killercup requested a review from a team July 12, 2019 08:46
@killercup

This comment has been minimized.

@JohnTitor
Copy link
Member

mem::initialized will be deprecated in 1.39.0, not 1.38.0 by this PR but it's reasonable to allow that before it is too late.

These were deprecated recently but we don't want to use them yet to not
break compatibility with older rustc versions unnecessary. Explicitly
allowing them should fix the build for nightly with `#[deny(warnings)]`.
@killercup
Copy link
Member Author

Rebased on top of your branch, @JohnTitor, so it now includes the Once::new(). Let's see what CI says.

@JohnTitor
Copy link
Member

Hmm, an error unrelated to this PR has occurred in macOS mysql beta.

@killercup
Copy link
Member Author

Yeah, the tests even passed but

Hosted Agent lost communication with the server

when running rustdoc.

I'll read this as "CI is green" :D

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

lgtm, nice work!

@sgrif
Copy link
Member

sgrif commented Jul 19, 2019

Given that many folks on the unsafe code guidelines feel that mem::uninitialized can never be used safely without UB, should we just switch to using MaybeUninit instead of allowing deprecated here? Or is it not stable on the current stable yet?

@JohnTitor
Copy link
Member

JohnTitor commented Jul 19, 2019

The problem with using MaybeUninit is that it has recently been stabilized (1.36.0). As mentioned above, we need to bump up the minimal version. If bumping up it isn't a big problem, it's good to replace uninitialized() with MaybeUninit.

@sgrif
Copy link
Member

sgrif commented Jul 19, 2019

Our policy is that we only bump if we have a good reason (e.g. not just for a convenience function). I think that meets this bar. We usually want to have some amount of lag so folks using their distro's Rust have a chance of being able to get the right version, but I think right now we're at least a few months from 2.0 shipping so it's fine.

@JohnTitor
Copy link
Member

Yeah, it seems we have a good reason to bump up the version.

@weiznich
Copy link
Member

weiznich commented Aug 6, 2019

I would vote for closing this PR.
#2131 fixes the CI by allowing all those warnings on nightly builds.
#2108 Fixes the warning regarding to ONCE_INIT
The deprecation of mem::uninitialized should be handled in a different PR

@killercup
Copy link
Member Author

👍

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