prefer if let to match with None => () arm in some places#34268
prefer if let to match with None => () arm in some places#34268bors merged 1 commit intorust-lang:masterfrom
if let to match with None => () arm in some places#34268Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
I think we do care. Everything looks good to me. Do r+ if Travis passes. (Here is bors documentation.) @bors delegate+ |
|
✌️ @zackmdavis can now approve this pull request |
src/libsyntax/print/pprust.rs
Outdated
There was a problem hiding this comment.
The body of this if statement is indented too much -- it should be:
if span.hi < (*cmnt).pos && (*cmnt).pos < next &&
span_line.line == comment_line.line {
self.print_comment(cmnt)?;
self.cur_cmnt_and_lit.cur_cmnt += 1;
}There was a problem hiding this comment.
@jseyfried Sure, I like that; I was just accepting the indentation Emacs gave me. Is this "when breaking up an if-condition onto multiple lines, the condition continuation line gets a four-space indent, but the body of the conditional only gets one additional space's worth of indent beyond that" rule documented anywhere? (I don't see anything in src/doc/style/style/whitespace.md.) Is so, this would be a bug (or desirable feature-request) in Emacs rust-mode.
There was a problem hiding this comment.
@zackmdavis I believe the rule is "when breaking up an if-condition onto multiple lines, the condition continuation line gets a three-space indent (so that it lines up with the first line of the if-condition), and the body is indented four spaces as usual".
I'm not sure if this is documented anywhere, but I've seen it a lot in the Rust code base. I've also seen other styles for the if-condition, like
if span.hi < (*cmnt).pos && (*cmnt).pos < next
&& span_line.line == comment_line.line {but the body should always be indented four spaces no matter what.
There was a problem hiding this comment.
three-space indent [...] body is indented four spaces as usual
Oh, of course; it looks like I miscounted last night (I was sleepy).
fc34b21 to
332dc05
Compare
|
|
src/librustc/middle/dead.rs
Outdated
There was a problem hiding this comment.
nit: this line and the following block are indented 5 spaces relative to the if let -- should be 4.
Agreed -- I almost always prefer a clean/meaningful history to a "historically accurate" history. |
332dc05 to
824d529
Compare
Casual grepping revealed some places in the codebase (some of which antedated `if let`'s December 2014 stabilization in c200ae5a) where we were using a match with a `None => ()` arm where (in the present author's opinion) an `if let` conditional would be more readable. (Other places where matching to the unit value did seem to better express the intent were left alone.) It's likely that we don't care about making such trivial, non-functional, sheerly æsthetic changes. But if we do, this is a patch.
824d529 to
8531d58
Compare
|
"Semantic indents are always four spaces; if a long if-condition must be broken onto multiple lines, the continuation line may receive either a three-space or eight-space indent, to taste"? |
|
Force-pushed again. My reading comprehension was not in best form last night, but I think the whitespace should be good now. |
|
Great! @bors r+ |
|
📌 Commit 8531d58 has been approved by |
…m, r=jseyfried prefer `if let` to match with `None => ()` arm in some places Casual grepping revealed some places in the codebase (some of which antedated `if let`'s December 2014 stabilization in c200ae5a) where we were using a match with a `None => ()` arm where (in the present author's opinion) an `if let` conditional would be more readable. (Other places where matching to the unit value did seem to better express the intent were left alone.) It's likely that we don't care about making such trivial, non-functional, sheerly æsthetic changes. But if we do, this is a patch.
This is a spiritual succesor to rust-lang#34268/8531d581, in which we replaced a number of matches of None to the unit value with `if let` conditionals where it was judged that this made for clearer/simpler code (as would be recommended by Manishearth/rust-clippy's `single_match` lint). The same rationale applies to matches of None to the empty block.
Casual grepping revealed some places in the codebase (some of which
antedated
if let's December 2014 stabilization in c200ae5a) where wewere using a match with a
None => ()arm where (in the presentauthor's opinion) an
if letconditional would be more readable. (Otherplaces where matching to the unit value did seem to better express the
intent were left alone.)
It's likely that we don't care about making such trivial,
non-functional, sheerly æsthetic changes.
But if we do, this is a patch.