Conversation
| extern crate arena; | ||
| #[macro_use] extern crate rustc; | ||
| #[macro_use] | ||
| extern crate rustc; |
There was a problem hiding this comment.
IMO, this is actually a good change, for consistency. Attributes usually go above statements. You'd never write:
#[inline] fn test() {
}There was a problem hiding this comment.
Good point.
I would put empty lines around the #[macro_use] extern crate line though. And re-order the imports so that the #[macro_use] ones come first.
There was a problem hiding this comment.
You mean like this?
#[macro_use]
extern crate rustc;
extern crate arena;
extern crate rustc_data_structures;
extern crate rustc_mir;
extern crate syntax;I agree that putting a newline before attributes makes sense.
However, re-ordering statements isn't rustfmt can reliably do due to comments. That is, there's no way to determine if // From rustc. should stick to extern crate arena; or if it should stay where it is.
There was a problem hiding this comment.
I'm mostly ambivalent about this change. I used the same-line #[macro_use] after seeing it in other projects including rustc. However, checking now, it's not even consistent within rustc.
On the other hand, I want to keep the list of crates alphabetical.
|
There are a bunch of good changes in here (a bunch of which I didn't comment on), but there's also a lot of stuff I consider harmful to readability and editability, like use of alignment for function arguments and multi-line method chains. I say it's harmful to readability because it rapidly increases rightward drift which leads to yet more line-breaking and alignment when the things being pushed right are already long. Altogether that can lead to way more lines for an originally compact piece of code. I say it's harmful to editability because alignment means you need to update a bunch of other lines when you make a change to something like the name of function that is called with aligned arguments. For me, either rustfmt defaults need to change or I'd have to adopt a custom configuration, which seems unfortunate. I'm pro-uniform formatting across the community, but rustfmt is currently a bit crazy. |
|
Oh, I should note as well that the original code is definitely not fully consistent, especially not function signatures. I was trying out a few different styles at different times, and now I think I lean towards a style I got from @retep998. For signatures that fit on line, nothing is done. For signatures that are too long, it becomes: fn assign_to_aggregate(
&mut self,
dest: Pointer,
dest_repr: &Repr,
variant: usize,
discr: Option<u64>,
operands: &[mir::Operand<'tcx>],
) -> EvalResult<()> {
// ...
}(NB: As it happens, though, |
For https://users.rust-lang.org/t/please-help-test-rustfmt/5386
I divided the changes that rustfmt into 4 sets: Good, Ambiguous, Bad, and Errors. All but the last are pretty subjective, I guess.
I myself would only go with the "Good Changes" (and error fixes, of course), but I included all changes so this PR can also be used for discussing some rustfmt stuff.