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.

Sign up for GitHub

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

Jump to bottom

Remove some unused functionality #92895

Merged
merged 2 commits into from
Feb 12, 2022
Merged

Remove some unused functionality #92895

merged 2 commits into from
Feb 12, 2022

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 14, 2022 •
edited
Loading

  • Remove the alt_std_name option
  • Make two functions private

@bjorn3 bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 14, 2022
@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 14, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camsteffen
Copy link
Contributor

  • Remove the everybody loops pass

I think this deserves a last laugh. teehee!

@bors
Copy link
Contributor

bors commented Jan 18, 2022

☔ The latest upstream changes (presumably #87648) made this pull request unmergeable. Please resolve the merge conflicts.

@lqd
Copy link
Member

lqd commented Jan 19, 2022

Why remove everybody loops btw, did it maybe stop working ? Isn't it still an interesting strategy for test case reduction, as documented by @pnkfelix in the classic minimization strategies blog post ?

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 20, 2022

It probably still works, but it is rather fragile (it has caused ice in the past) and it removes def id's after expansion, which requires various places to take this into account (eg https://github.com/rust-lang/rust/pull/92895/files#diff-b27a380db7c2a077975b81b162d8a7923c599d56b4f265427f2a9eecc77641b4L85-L86) My primary reason however was to reduce compiler complexity a bit.

Isn't it still an interesting strategy for test case reduction, as documented by @pnkfelix in the classic minimization strategies blog post ?

Rust-reduce has a pass that does something similar: https://github.com/jethrogb/rust-reduce/blob/master/src/transforms/clear_blocks.rs

@lqd
Copy link
Member

lqd commented Jan 20, 2022

Rust-reduce has a pass that does something similar: https://github.com/jethrogb/rust-reduce/blob/master/src/transforms/clear_blocks.rs

Sure, I know, I use it often. So it doesn't affect me that much but not everyone who uses everybody_loops does, or can, use rust-reduce, hence why I was asking.

Probably not a big deal, of course, let's see what the others think.

@bors
Copy link
Contributor

bors commented Jan 22, 2022

☔ The latest upstream changes (presumably #93173) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 22, 2022

I will hold off on rebasing until a decision is made if everybody_loops should be removed or not.

@jackh726
Copy link
Member

cc @pnkfelix for thoughts on everybody loops

@jackh726
Copy link
Member

Actually let's just ping @rust-lang/compiler for thought

@nagisa
Copy link
Member

nagisa commented Jan 30, 2022

I have personally always found -Zeverybody-loops an oversized hammer. It only helps in reducing issues in very limited set of rustc components early in the pipeline (parser, resolver…). An ability to loopify bodies more selectively is definitely a technique that is helpful still, but -Zeverybody-loops ain't that.

If the pass causes genuine pain maintaining other parts of the codebase, I'm probably in favour of removing it. That said, I would also like to suggest to split out the removal of -Zeverybody-loops into a separate, well motivated PR, rather than bundling it together with other more obvious removals.

@jackh726
Copy link
Member

Given the above comment and thumbs ups, r=me after rebase

@jackh726 jackh726 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2022
@jackh726
Copy link
Member

Err, well. Let's split this into two. r=me on the non everybody-loops change.

@bjorn3
Remove the alt_std_name option
55ceed8 
This option introduced in rust-lang#15820 allows a custom crate to be imported in
the place of std, but with the name std. I don't think there is any
value to this. At most it is confusing users of a driver that uses this option. There are no users of
this option on github. If anyone still needs it, they can emulate it
injecting #![no_core] in addition to their own prelude.
@bjorn3
Make two functions private
7ba4110
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 11, 2022

Dropped the everybody loops removal from this PR. Will make a separate PR for it shortly.

@bors r=jackh726

@bors
Copy link
Contributor

bors commented Feb 11, 2022

📌 Commit 7ba4110 has been approved by jackh726

@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 Feb 11, 2022
@bjorn3 bjorn3 mentioned this pull request Feb 11, 2022
Merged
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 11, 2022

Split out the everybody loops removal to #93913.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2022
@matthiaskrgr
Rollup merge of rust-lang#92895 - bjorn3:simplifications, r=jackh726
9c6b8c7 
Remove some unused functionality

* Remove the `alt_std_name` option
* Remove the everybody loops pass
* Make two functions private
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 11, 2022
@matthiaskrgr
Rollup merge of rust-lang#92895 - bjorn3:simplifications, r=jackh726
a72c0e1 
Remove some unused functionality

* Remove the `alt_std_name` option
* Remove the everybody loops pass
* Make two functions private
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2022
Closed
@klensy
Copy link
Contributor

klensy commented Feb 11, 2022

Split out the everybody loops removal to #93913.

PR description not changed.

@matthiaskrgr matthiaskrgr mentioned this pull request Feb 11, 2022
Merged
@bjorn3
Copy link
Member Author

bjorn3 commented Feb 11, 2022

My bad. Fixed that now. The merge commit may not reflect it though if the current rollup succeeds.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2022
@bors
Auto merge of rust-lang#93921 - matthiaskrgr:rollup-wn3jlxj, r=matthi…
f198510 
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#90955 (Rename `FilenameTooLong` to `InvalidFilename` and also use it for Windows' `ERROR_INVALID_NAME`)
 - rust-lang#91607 (Make `span_extend_to_prev_str()` more robust)
 - rust-lang#92895 (Remove some unused functionality)
 - rust-lang#93635 (Add missing platform-specific information on current_dir and set_current_dir)
 - rust-lang#93660 (rustdoc-json: Add some tests for typealias item)
 - rust-lang#93782 (Split `pauth` target feature)
 - rust-lang#93868 (Fix incorrect register conflict detection in asm!)
 - rust-lang#93888 (Implement `AsFd` for `&T` and `&mut T`.)
 - rust-lang#93909 (Fix typo: explicitely -> explicitly)
 - rust-lang#93910 (fix mention of moved function in `rustc_hir` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 642414e into rust-lang:master Feb 12, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 12, 2022
@bjorn3 bjorn3 deleted the simplifications branch February 12, 2022 11:01
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 4, 2022
@Dylan-DPC
Rollup merge of rust-lang#93913 - bjorn3:remove_everybody_loops, r=ja…
0a67c2f 
…ckh726

Remove the everybody loops pass

It isn't used anymore by rustdoc.

Split out of rust-lang#92895. There has been some previous discussion there.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 4, 2022
@Dylan-DPC
Rollup merge of rust-lang#93913 - bjorn3:remove_everybody_loops, r=ja…
c1585a1 
…ckh726

Remove the everybody loops pass

It isn't used anymore by rustdoc.

Split out of rust-lang#92895. There has been some previous discussion there.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jackh726 jackh726

Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
10 participants
@bjorn3 @rust-highfive @rust-log-analyzer @camsteffen @bors @lqd @jackh726 @nagisa @klensy @rustbot