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

Transition liballoc to Rust 2018 #58081

Merged
merged 12 commits into from
Feb 3, 2019
Merged

Transition liballoc to Rust 2018 #58081

merged 12 commits into from
Feb 3, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Feb 2, 2019

This transitions liballoc to Rust 2018 edition and applies relevant idiom lints.
I also did a small bit of drive-by cleanup along the way.

r? @oli-obk

I started with liballoc since it seemed easiest. In particular, adding edition = "2018" to libcore gave me way too many errors due to stdsimd. Ideally we should be able to continue this crate-by-crate until all crates use 2018.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 2, 2019
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

This looks generally good overall -- I'm a bit concerned we might be losing some of the bounds on some impls with regards to lifetimes -- but presuming those are all inferred anyway then seems fine.

I would personally prefer 2018-related changes separate from the formatting and cleanup, but don't care too much I guess.

src/liballoc/alloc.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@shepmaster
Copy link
Member

relevant idiom lints

Out of curiosity, which did you use?

@Centril
Copy link
Contributor Author

Centril commented Feb 3, 2019

@shepmaster I enabled:

#![deny(rust_2018_idioms)]
#![allow(explicit_outlives_requirements)]

since we might want to leave some things inferred but not others, and then I fixed some errors.

@Centril
Copy link
Contributor Author

Centril commented Feb 3, 2019

Reverted most of the nested import style changes.

As for the <'_> changes that aren't necessary due to rust_2018_idioms, I think they should stay because they are an important part of the 2018 edition story around ergonomics and the rust repo shouldn't intentionally attempt to look unlike idiomatic Rust code.

@SimonSapin
Copy link
Contributor

SimonSapin commented Feb 3, 2019

This might not be the place to discuss this, but I personally mildly dislike adding <'_> to every type with a lifetime parameter that was inferred. I am opposed to doing so in cases where that type is already behind a borrow. A common case of the latter is fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 3, 2019

As for the <'_> changes that aren't necessary due to rust_2018_idioms, I think they should stay because they are an important part of the 2018 edition story around ergonomics and the rust repo shouldn't intentionally attempt to look unlike idiomatic Rust code.

Anything we aren't enforcing via lints will get forgotten and thus become inconsistent. I don't have a strong opinion either way, but I really want this PR merged. Can you remove the unnecessary <'_> additions (like those in Formatter<'_>) until it's uncontroversial and lint-enforcable?

@Centril
Copy link
Contributor Author

Centril commented Feb 3, 2019

@oli-obk

Anything we aren't enforcing via lints will get forgotten and thus become inconsistent. I don't have a strong opinion either way, but I really want this PR merged. Can you remove the unnecessary <'_> additions (like those in Formatter<'_>) until it's uncontroversial and lint-enforcable?

The Formatter<'_> thing is lint enforced by #![deny(rust_2018_idioms)]. The bits that aren't enforced are things like impl<T> Iterator for Drain<'_, T> { which was introduced with stabilizing impl_header_lifetime_elision (see edition guide and stabilization proposal). This should be uncontroversial since otherwise it wouldn't have been stabilized. :) (edit: I don't mean to say that you must write impl ... Drain<'_, T>... I only used it conservatively in a few places I thought it made the code nicer...)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 3, 2019

Oh yea, moving from explicit lifetimes to inferred once is definitely a gain. Since the idoom lints are enforcing the other lifetimes, I agree that they are uncontroversial until proven otherwise (by writing RFCs and changing the lints).

@bors r+

@bors
Copy link
Contributor

bors commented Feb 3, 2019

📌 Commit 2396780 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 3, 2019
@bors
Copy link
Contributor

bors commented Feb 3, 2019

⌛ Testing commit 2396780 with merge 4f4f4a4...

bors added a commit that referenced this pull request Feb 3, 2019
Transition liballoc to Rust 2018

This transitions liballoc to Rust 2018 edition and applies relevant idiom lints.
I also did a small bit of drive-by cleanup along the way.

r? @oli-obk

I started with liballoc since it seemed easiest. In particular, adding `edition = "2018"` to libcore gave me way too many errors due to stdsimd. Ideally we should be able to continue this crate-by-crate until all crates use 2018.
@bors
Copy link
Contributor

bors commented Feb 3, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 4f4f4a4 to master...

@bors bors merged commit 2396780 into rust-lang:master Feb 3, 2019
@Centril Centril deleted the liballoc-2018 branch February 14, 2019 02:54
clint-white added a commit to clint-white/binary-heap-plus-rs that referenced this pull request Aug 7, 2022
This commit ports rust-lang/rust commit
e70c2fbd5cbe8bf176f1ed01ba9a53cec7e842a5 "liballoc: elide some
lifetimes".  Part of rust-lang/rust#58081: Transition liballoc to Rust
2018.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants