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

compiler: clippy::complexity fixes #121523

Merged
merged 1 commit into from
Feb 24, 2024
Merged

Conversation

matthiaskrgr
Copy link
Member

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2024

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Comment on lines -558 to -564
fn count<'a>(
cx: &ExtCtxt<'a>,
depth_curr: usize,
depth_max: usize,
matched: &NamedMatch,
sp: &DelimSpan,
) -> PResult<'a, usize> {
Copy link
Member Author

Choose a reason for hiding this comment

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

cx and sp were only used in recursion and thus removed

Copy link
Member

Choose a reason for hiding this comment

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

wow that's a cool lint!

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, those are some nice improvements

Comment on lines -558 to -564
fn count<'a>(
cx: &ExtCtxt<'a>,
depth_curr: usize,
depth_max: usize,
matched: &NamedMatch,
sp: &DelimSpan,
) -> PResult<'a, usize> {
Copy link
Member

Choose a reason for hiding this comment

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

wow that's a cool lint!

@@ -555,9 +555,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
{
let args = self.infcx.fresh_args_for_item(call_name.span, assoc.def_id);
let fn_sig = tcx.fn_sig(assoc.def_id).instantiate(tcx, args);
let fn_sig =
self.instantiate_binder_with_fresh_vars(call_name.span, FnCall, fn_sig);
Some((assoc, fn_sig));
Copy link
Member

Choose a reason for hiding this comment

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

lol

Copy link
Member

@lukas-code lukas-code Feb 23, 2024

Choose a reason for hiding this comment

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

It looks like this entire similar_assoc closure doesn't actually do anything except return None? It should probably be fixed or removed entirely.

return Some((assoc, fn_sig)); seems to be what the author intended here.

&& args.len() >= parent_args.len() + 1
&& args.len() > parent_args.len()
Copy link
Member

Choose a reason for hiding this comment

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

wtf

&& !(sess.codegen_units().as_usize() == 1)
&& (sess.codegen_units().as_usize() != 1)
Copy link
Member

Choose a reason for hiding this comment

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

amazing

Comment on lines -1034 to +1035
} else if obligation.predicate.args.type_at(0).to_opt_closure_kind().is_some()
&& obligation.predicate.args.type_at(1).to_opt_closure_kind().is_some()
{
true
} else {
false
obligation.predicate.args.type_at(0).to_opt_closure_kind().is_some()
Copy link
Member

Choose a reason for hiding this comment

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

not sure whether this is really better 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I think if cond { true } else { false } is kinda silly 😅

Copy link
Member

Choose a reason for hiding this comment

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

alone yes, but in an else if chain, keeping it consistent can be nicer

@Noratrieb
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 23, 2024

📌 Commit 86a7fc8 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 Feb 23, 2024
@@ -555,9 +555,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
{
let args = self.infcx.fresh_args_for_item(call_name.span, assoc.def_id);
let fn_sig = tcx.fn_sig(assoc.def_id).instantiate(tcx, args);
let fn_sig =
self.instantiate_binder_with_fresh_vars(call_name.span, FnCall, fn_sig);
Some((assoc, fn_sig));
Copy link
Member

@lukas-code lukas-code Feb 23, 2024

Choose a reason for hiding this comment

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

It looks like this entire similar_assoc closure doesn't actually do anything except return None? It should probably be fixed or removed entirely.

return Some((assoc, fn_sig)); seems to be what the author intended here.

@@ -1264,7 +1264,7 @@ fn validate_commandline_args_with_session_available(sess: &Session) {
// LLVM CFI using rustc LTO requires a single codegen unit.
if sess.is_sanitizer_cfi_enabled()
&& sess.lto() == config::Lto::Fat
&& !(sess.codegen_units().as_usize() == 1)
&& (sess.codegen_units().as_usize() != 1)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
&& (sess.codegen_units().as_usize() != 1)
&& sess.codegen_units().as_usize() != 1

Why doesn't the unused_parens lint trigger here?

Copy link
Member

Choose a reason for hiding this comment

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

the lint avoids triggering when mixed-precedence operators are involved because it can be useful to clarify them explicitly. here it's a bit silly though, yeah.

@bors
Copy link
Contributor

bors commented Feb 23, 2024

⌛ Testing commit 86a7fc8 with merge 7c3e305...

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
@bors
Copy link
Contributor

bors commented Feb 23, 2024

💔 Test failed - checks-actions

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

@bors retry
Error response from daemon: Get "https://ghcr.io/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

@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 23, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2024
@bors
Copy link
Contributor

bors commented Feb 23, 2024

⌛ Testing commit 86a7fc8 with merge 9c66ce7...

@bors
Copy link
Contributor

bors commented Feb 23, 2024

💔 Test failed - checks-actions

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

@bors treeclosed=50

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
2
Image input checksum 464bbf01216979434d2a03f74922b680ae69a026f44246e23063db550f2ae7d04f59432b522590b9a47bc18ee1b0ac5f5fc572c3e2247601a6775637a4935ea9
##[group]Building docker image for dist-x86_64-linux
Docker version 24.0.8, build e0dfb46
Error response from daemon: Get "https://ghcr.io/v2/": net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)
##[error]Process completed with exit code 1.

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
7e554bd1e09e: Pushed
d101c9453715: Pushed
3a737237aa0b: Pushed
1b06dfedb026: Pushed
Put "https://ghcr.io/v2/rust-lang-ci/rust-ci/manifests/926e201d94395afbcb23699128884ff38ac276381245aa9c137fe5edbdf2a39b52d33c56b54b58497aa7441ab8c466100c4edfbfb71dea043669b141fdf0719c": dial tcp 140.82.112.34:443: i/o timeout
##[error]Process completed with exit code 1.

@matthiaskrgr
Copy link
Member Author

@bors retry p=51

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2024
@bors
Copy link
Contributor

bors commented Feb 24, 2024

⌛ Testing commit 86a7fc8 with merge 341edc2...

@matthiaskrgr
Copy link
Member Author

@bors treeclosed-
seems to be working..?

@bors
Copy link
Contributor

bors commented Feb 24, 2024

💥 Test timed out

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

new exciting failure!
@bors retry

@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 24, 2024
@Noratrieb
Copy link
Member

@bors p=0

@bors
Copy link
Contributor

bors commented Feb 24, 2024

⌛ Testing commit 86a7fc8 with merge 6bdb8a4...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@matthiaskrgr
Copy link
Member Author

2024-02-24T15:19:00.6213164Z test net::tcp::tests::close_read_wakes_up has been running for a long time
2024-02-24T19:07:34.6641649Z Entering debug mode. Use h or ? for help.
2024-02-24T19:07:34.6644979Z
2024-02-24T19:07:34.7483266Z At C:\a\rust\rust\x.ps1:20 char:5
2024-02-24T19:07:34.7483728Z + Exit $LASTEXITCODE
2024-02-24T19:07:34.7484149Z + ~~~~~~~~~~~~~~~~~~
2024-02-24T19:07:35.4677579Z [DBG]: PS C:\a\rust\rust>>
2024-02-24T19:07:35.4809548Z ##[error]The operation was canceled.
2024-02-24T19:07:35.5425355Z Post job cleanup.

:|

@bors
Copy link
Contributor

bors commented Feb 24, 2024

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 6bdb8a4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 24, 2024
@bors bors merged commit 6bdb8a4 into rust-lang:master Feb 24, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 24, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6bdb8a4): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)
1.5% [0.2%, 2.7%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.2% [-1.2%, -1.2%] 1
All ❌✅ (primary) 1.5% [0.2%, 2.7%] 2

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

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)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
4.5% [4.4%, 4.8%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) 2.7% [2.7%, 2.7%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 648.482s -> 649.823s (0.21%)
Artifact size: 310.98 MiB -> 311.02 MiB (0.01%)

@rylev
Copy link
Member

rylev commented Feb 27, 2024

The large regression that caused this to be marked overall as a regression is just noise.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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