collapse nested if blocks

[danieleades]

No description provided.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[alexcrichton]

Personally I prefer to not blanket do this via the guidance of some external tool. The structure of if/else often reflects the evolution of the code over time and I don't think that collapsing isolated blocks is always a net benefit. In some situations it hinders future refactoring or other future tweaks. Bunching all if conditions together I also find pretty unreadable sometimes.

[danieleades]

Personally I prefer to not blanket do this via the guidance of some external tool. The structure of if/else often reflects the evolution of the code over time and I don't think that collapsing isolated blocks is always a net benefit. In some situations it hinders future refactoring or other future tweaks. Bunching all if conditions together I also find pretty unreadable sometimes.

Personally i don't think collapsing the statements makes them less readable, though it's debatable whether it makes them more readable in all cases. It does remove a level of indentation. Also, if another else/if else condition is added later to the same block, there can be ambiguity about whether to add that the the outer or inner scope of the nested statements. it's a small risk, but you eliminate it by collapsing the blocks.

i definitely think collapsing the "if { else {...}}" blocks into "if else {...}" blocks improves readability. there's a couple of examples of that.

there are some ugly ones in there too, for example

 if unit.target.is_cdylib()
                    && !self.compilation.cdylibs.iter().any(|uo| uo.unit == *unit) {
        ...
}

but i don't think this is any less readable than the original-

 if unit.target.is_cdylib() {
                    if !self.compilation.cdylibs.iter().any(|uo| uo.unit == *unit) {
                          ...
                    }

It might be worth naming some of this checks to document what it is they're actually testing. as in,

// as a free function, or a struct method...
fn check_that_thing(cdylibs: ...) -> bool {
   ...
}

 if unit.target.is_cdylib() && check_that_thing(self.compilation.cdylibs) {
    ...
}

[ehuss]

I'm going to go ahead and merge this, but I want to point out that there is a comment here specifically saying we would prefer to not have PRs fixing clippy lints. They tend to be subjective, or have false positives, or break things. In this case, I don't think this is a significant improvement, but a small reduction in nesting I think is fine.

@bors r+

[bors]

📌 Commit c5da878 has been approved by ehuss

[bors]

⌛ Testing commit c5da878 with merge e928b62...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing e928b62 to master...