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

collaps if x {} else { if .. } conditions #80793

Closed
wants to merge 1 commit into from

Conversation

matthiaskrgr
Copy link
Member

Fixes clippy::collapsible_else_if

reduces nesting of

if x {
 A
} else {
 if y {
  B
 } else {
  C
 }
}

to

if x {
 A
} else if y {
 B
} else {
 C
}

Fixes clippy::collapsible_else_if

reduces nesting of
````
if x {
 A
} else {
 if y {
  B
 } else {
  C
 }
}
````
to
````
if x {
 A
} else if y {
 B
} else
 C
}
````
@rust-highfive
Copy link
Collaborator

Some changes occurred in intra-doc-links.

cc @jyn514

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 7, 2021
@jyn514 jyn514 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 7, 2021
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Rustdoc changes LGTM

cmd.args(args);
}
} else if let Some(args) = sess.target.late_link_args_static.get(&flavor) {
cmd.args(args);
}
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I feel like this change is hiding intent - previously the code was pretty clearly handling the dynamic and non-dynamic cases, but now it is less obvious IMO, at least for me.

Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

I'm slightly concerned about reshaping this hiding intent in some cases, but the code changes are all functionally the same.

What was the trigger to look into cleaning up this pattern in the codebase?

I'm r=me but see multiple places where I wish we had more comments.

} else if !args.named_args.is_empty() || !args.reg_args.is_empty() {
let mut err = ecx.struct_span_err(
span,
"positional arguments cannot follow named arguments \
or explicit register arguments",
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation

@bors
Copy link
Contributor

bors commented Jan 12, 2021

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

@JohnCSimon
Copy link
Member

Ping from triage
@matthiaskrgr - can you please address the merge conflicts? This already has approvals. Thank you.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot 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 1, 2021
@matthiaskrgr
Copy link
Member Author

I'm closing this in regard to the criticism regarding readability.

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. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

8 participants