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

Force to use block for closure body with a single control flow expression #1877

Merged
merged 6 commits into from
Nov 3, 2017

Conversation

topecongiro
Copy link
Contributor

This PR forces rustfmt to use block when rewriting the body of closure with a single control flow expression.
CC #1791, rust-lang/style-team#35.

1
} else {
2
loong_func().quux(move || {
Copy link
Contributor

@marcusklaas marcusklaas Aug 12, 2017

Choose a reason for hiding this comment

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

This looks funny. Isn't the indentation too deep here?

Edit: nope - it's fine. Tabs just render really wide on my machine.

@nrc
Copy link
Member

nrc commented Aug 13, 2017

Thanks for this PR. I might leave this open for a while until we've decided on what the behaviour ought to be (see rust-lang/style-team#35 (comment)). Is that ok?

let index = *first as usize;
if index >= ENCODINGS.len() {
return;
}
}
let encoding = ENCODINGS[index];
dispatch_test(encoding, &data[1..]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did these two lines move from inside the if let to outside it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a regression. Thank you for pointing out.

@topecongiro
Copy link
Contributor Author

Thanks for this PR. I might leave this open for a while until we've decided on what the behaviour ought to be (see rust-lang/style-team#35 (comment)). Is that ok?

@nrc That would be perfectly fine with me.

@topecongiro topecongiro force-pushed the overflowing-closure-with-loop branch 2 times, most recently from 64901fe to a42f9f2 Compare August 28, 2017 01:20
@topecongiro
Copy link
Contributor Author

@nrc Could you share how you feel about this PR now?

@nrc
Copy link
Member

nrc commented Nov 2, 2017

Hmm, there wasn't much movement there and I don't have a strong preference myself. Lets land this, I think. Better to err on the side of using a block (some of the examples without looked a bit odd).

Are you OK to rebase?

@topecongiro
Copy link
Contributor Author

@nrc Rebased. Could you please review it?

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Looks good, just needs that one comment, then r+

src/expr.rs Outdated
@@ -2310,6 +2302,22 @@ where
}
}

fn args_have_many_closure<T>(args: &[&T]) -> bool
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment describing what this function does please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Added a comment.

@nrc nrc merged commit f15dd87 into rust-lang:master Nov 3, 2017
@nrc
Copy link
Member

nrc commented Nov 3, 2017

Thanks!

@topecongiro topecongiro deleted the overflowing-closure-with-loop branch November 3, 2017 04:24
@shepmaster
Copy link
Member

Thank you @topecongiro; if I understand what the change does, this has been driving me batty for a while now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants