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

Remove some ref patterns from the compiler #106090

Merged
merged 12 commits into from
Jan 20, 2023

Conversation

WaffleLapkin
Copy link
Member

Previous PR: #105368

r? @Nilstrieb

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 23, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 23, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

Thanks, most of those look way better now! One let_else fomatting fix (really excited for the day when rustfmt will finally work with it :D) and a nit that you may or may not address, whatever you prefer.
r=me then

compiler/rustc_const_eval/src/interpret/operand.rs Outdated Show resolved Hide resolved
compiler/rustc_const_eval/src/transform/promote_consts.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Dec 29, 2022

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

@Noratrieb Noratrieb 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 Jan 3, 2023
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 12, 2023

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the dereffffffffff branch 2 times, most recently from 9d1af0b to 5d1202b Compare January 12, 2023 18:39
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Jan 12, 2023

I think I've addressed all review comments. @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2023
@bors
Copy link
Contributor

bors commented Jan 13, 2023

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

@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 13, 2023
@bors
Copy link
Contributor

bors commented Jan 13, 2023

⌛ Trying commit 93f40f77ea04d0f59f10f0dda242814e4df5cc44 with merge 5cb9c9d58b4d1ad3f602047be2697d7569345deb...

Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

A few more comments, but looking good overall

compiler/rustc_const_eval/src/interpret/operand.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/check/compare_impl_item.rs Outdated Show resolved Hide resolved
}
_ => bug!("{:?} is not a method", impl_m),
let mut impl_args = {
let ImplItemKind::Fn(sig, _) = &tcx.hir().expect_impl_item(impl_m.def_id.expect_local()).kind else { bug!("{:?} is not a method", impl_m) };
Copy link
Member

Choose a reason for hiding this comment

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

All of these bugs make me think that we should have expect_* methods on ImplItemKind - but that's something for the future

}
}

_ => ty.try_super_fold_with(self),
})()?;
Copy link
Member

Choose a reason for hiding this comment

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

Very weird, I hope there wasn't something weird going on, but the change looks correct🤨. Well, there are quite a few of those around, I guess it's just an artifact of refactorings and made sense at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, when I was removing these I was triple checking in case I've missed something ><

@Noratrieb Noratrieb 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 Jan 13, 2023
@bors
Copy link
Contributor

bors commented Jan 13, 2023

☀️ Try build successful - checks-actions
Build commit: 5cb9c9d58b4d1ad3f602047be2697d7569345deb (5cb9c9d58b4d1ad3f602047be2697d7569345deb)

WaffleLapkin and others added 7 commits January 17, 2023 07:48
rustfmt, pleaaaaase, start supporting rust

Co-authored-by: nils <48135649+Nilstrieb@users.noreply.github.com>
- add back accidentally removed new lines
- try to deref in patterns, rather than in expressions
  (maybe this was the reason of perf regression?...)
@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 17, 2023
@WaffleLapkin
Copy link
Member Author

@bors try
The CI failure should be fixed, but I can't test this locally.

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jan 17, 2023
@WaffleLapkin WaffleLapkin removed the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jan 17, 2023
@WaffleLapkin
Copy link
Member Author

The builder that was failing now passes, so I think this should be ready to be merged.

Comment on lines +115 to +121

// Skip over '{' at the start of the tail, so we don't later wrongfully consider this as json.
// See <https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Weird.20CI.20failure/near/321797811>
while tail.get(0) == Some(&b'{') {
tail = &tail[1..];
skipped += 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

this is, uh, certainly something. fun that no one has hit this before, you were the lucky one!

@Noratrieb
Copy link
Member

That was quite a PR, as I said already it would be nice if it was better split-up next time. But anyways, should be good to go now!
@bors r+

@bors
Copy link
Contributor

bors commented Jan 19, 2023

📌 Commit 65d1e8d has been approved by Nilstrieb

It is now in the queue for this repository.

@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 Jan 19, 2023
@bors
Copy link
Contributor

bors commented Jan 20, 2023

⌛ Testing commit 65d1e8d with merge 56ee852...

@bors
Copy link
Contributor

bors commented Jan 20, 2023

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 56ee852 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 20, 2023
@bors bors merged commit 56ee852 into rust-lang:master Jan 20, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 20, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (56ee852): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.2% [-0.2%, -0.1%] 3
Improvements ✅
(secondary)
-0.3% [-0.5%, -0.3%] 5
All ❌✅ (primary) -0.2% [-0.2%, -0.1%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.1% [1.9%, 2.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.2% [-1.2%, -1.2%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.3% [-3.3%, -3.3%] 1
All ❌✅ (primary) - - 0

@WaffleLapkin WaffleLapkin deleted the dereffffffffff branch January 20, 2023 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants