Skip to content

rustdoc: Remove Clean impls for tuples #91501

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 11 commits into from
Dec 4, 2021
Merged

Conversation

camelid
Copy link
Member

@camelid camelid commented Dec 3, 2021

This PR removes all nine Clean impls on tuples, converting them to
functions instead.

The fact that these are impls causes several problems:

  1. They are nameless, so it's unclear what they do.

  2. It's hard to find where they're used apart from removing them and
    seeing what errors occur (this applies to all Clean impls, not just
    the tuple ones).

  3. Rustc doesn't currently warn when impls are unused, so dead code
    can accumulate easily (all Clean impls).

  4. Their bodies often use tuple field indexing syntax (e.g., self.1)
    to refer to their "arguments", which makes reading the code more
    difficult.

As I noted, some of these problems apply to all Clean impls, but even
those problems are exacerbated by the tuple impls since they make
general understanding of the code harder.

Converting the impls to functions solves all four of these problems.

r? @GuillaumeGomez

This commit removes the first of nine Clean impls on tuples, converting
it to a function instead.

The fact that these are impls causes several problems:

  1. They are nameless, so it's unclear what they do.

  2. It's hard to find where they're used apart from removing them and
     seeing what errors occur (this applies to all Clean impls, not just
     the tuple ones).

  3. Rustc doesn't currently warn when impls are unused, so dead code
     can accumulate easily (all Clean impls).

  4. Their bodies often use tuple field indexing syntax (e.g., `self.1`)
     to refer to their "arguments", which makes reading the code more
     difficult.

As I noted, some of these problems apply to all Clean impls, but even
those problems are exacerbated by the tuple impls since they make
general understanding of the code harder.

Converting the impls to functions solves all four of these problems.
This was the last one!
@camelid camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 3, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2021
@camelid
Copy link
Member Author

camelid commented Dec 3, 2021

This PR is best reviewed

  • commit-by-commit and
  • with "Show whitespace changes" turned off.

@camelid
Copy link
Member Author

camelid commented Dec 3, 2021

I made sure each removal passed x check rustdoc before committing, which means that all the use sites of the particular impl that was removed should have been fixed; so each commit should be reviewable on its own.

@GuillaumeGomez
Copy link
Member

This is really great, thanks a lot for this clean up!

@bors: r+

@bors
Copy link
Collaborator

bors commented Dec 3, 2021

📌 Commit e36561d has been approved by GuillaumeGomez

@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 Dec 3, 2021
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Dec 3, 2021

Woups, didn't see that CI wasn't done yet. r=me once CI pass.

@bors: r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 3, 2021
@camelid
Copy link
Member Author

camelid commented Dec 3, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Collaborator

bors commented Dec 3, 2021

📌 Commit e36561d has been approved by GuillaumeGomez

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 3, 2021
@camelid
Copy link
Member Author

camelid commented Dec 3, 2021

Could have tricky conflicts: @bors rollup=iffy

@bors
Copy link
Collaborator

bors commented Dec 3, 2021

⌛ Testing commit e36561d with merge 14c1e71...

@bors
Copy link
Collaborator

bors commented Dec 4, 2021

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 14c1e71 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 4, 2021
@bors bors merged commit 14c1e71 into rust-lang:master Dec 4, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 4, 2021
@camelid camelid deleted the rm-tuple-impls-2 branch December 4, 2021 03:03
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (14c1e71): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants