Rustfmt-ing librustc_mir. #29076
Rustfmt-ing librustc_mir. #29076
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
| fields.into_iter() | ||
| .map(|f| (f.name, unpack!(block = this.as_operand(block, f.expr)))) | ||
| .collect(); | ||
| let fields_map: FnvHashMap<_, _> = fields.into_iter() |
There was a problem hiding this comment.
@nrc so I was looking more closely here...I would say this is a definite regression in readability. This is a tough heuristic (non-local), but certainly when I choose where to put a new line, I have a kind of "global optimization" over the entire function call chain going on, and I feel like it applies here. Is there any kind of open bug on this scenario? Do you even see what I'm pointing out :)
There was a problem hiding this comment.
@nrc in particular, I'm optimizing largely for keeping the inner expressions readable, I think, which is where this formatting fails
There was a problem hiding this comment.
Not @nrc but this is certainly something I've been thinking about. One of the more simple heuristics we could start with is comparing the result of formatting on the same line and breaking after the assignment and choosing the one with the fewest line breaks (no break on tie).
There was a problem hiding this comment.
Yeah, it's something I've seen. I agree it is sub-optimal. I think the simple idea of comparing with or without line breaks after the = in assignments is probably a good idea - the majority of times I see this is in a let statement, so it would probably get us 90% of the way there. I worry a bit about combinatorial explosion - we'd want to do the check in as few places as possible.
There was a problem hiding this comment.
One of the more simple heuristics we could start with is comparing the result of formatting on the same line and breaking after the assignment and choosing the one with the fewest line breaks (no break on tie).
the majority of times I see this is in a let statement, so it would probably get us 90% of the way there
yes to both
There was a problem hiding this comment.
@marcusklaas do you know if we have an issue open for this?
There was a problem hiding this comment.
| }) | ||
| }) | ||
| .map(|(arm_index, pattern, guard)| { | ||
| Candidate { | ||
| match_pairs: vec![self.match_pair(discriminant_lvalue.clone(), pattern)], |
There was a problem hiding this comment.
The indentation here is wrong; should be relative to Candidate {.
| cx.tcx.region_maps.lookup_code_extent(remainder_extent); | ||
| let remainder_extent = cx.tcx | ||
| .region_maps | ||
| .lookup_code_extent(remainder_extent); |
There was a problem hiding this comment.
another case where the = heuristic ought to be used. I'd prefer to revert.
There was a problem hiding this comment.
In this case I actually prefer the new version, although I bet a simple = heuristic would choose the former. I prefer it because scanning the elements in a chain of method calls is useful, but there is no aid in scanning code in having a line break after =.
|
@goyox86 There's a lot of discussion on this one and a lot of places where there are bugs in rustfmt. It might be worth waiting for rustfmt to get better and then trying this again, rather than fixing up what is here. On the other hand, you could try to cherry pick the non-controversial changes and do some of the fixups (like moving comments) to make a smaller PR which could land. Up to you. |
|
@nrc Gonna close this one and wiat for rustfmt to get smarter ;) |
Hi Rustaceans!
This is the result of running latest rustfmt on librustc_mir!
//cc @nrc