Skip to content

run rustfmt on libtest folder #34085

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

Merged
merged 1 commit into from
Jun 6, 2016
Merged

run rustfmt on libtest folder #34085

merged 1 commit into from
Jun 6, 2016

Conversation

srinivasreddy
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Contributor

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Jun 5, 2016

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 5, 2016

📌 Commit 8a6a9af has been approved by eddyb

@@ -11,7 +11,7 @@
#![allow(missing_docs)]
#![allow(deprecated)] // Float

use std::cmp::Ordering::{self, Less, Greater, Equal};
use std::cmp::Ordering::{self, Equal, Greater, Less};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Less, Equal, Greater? That makes the most sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

That's not alphabetical, which is how rustfmt appears to be sorting it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but I disagree with rustfmt on this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the rule can be generalized to all enums by sorting variants by declaration order. This has the added benefit of inheriting any grouping that the author put in. Taking a few random examples from MIR to show how this rule would provide benefit:

  • rustc::mir::repr::TerminatorKind has a few intuitive groups. Goto, If, Switch, and SwitchInt are basic branches, going to one of a list of possible targets based on a simple condition. Resume and Return both signal exiting from some sort of scope, and Drop, DropAndReplace, Call, and Assert are all not inherently terminators, more like statements that can panic. By sorting by definition order, those groups would all be imported next to each other.
  • rustc::mir::repr::Lvalue is similarly grouped: Var, Temp, and Arg are function-local, Static and ReturnPointer represent externally created variables, and Projection is its own thing.
  • rustc::mir::repr::BorrowKind does not really have a division into groups. However, it is ordered by increasing permissions: Shared pointers can't be guaranteed unique access and can't mutate, Unique pointers form the in-between of unique access without mutability, and Mut pointers have all the permissions. Importing them in this order makes sense because it is the natural order to view the options in.

Essentially, developers already sort their enums in a logical, helpful way, and it makes sense to reuse that for import order.

Copy link
Member

Choose a reason for hiding this comment

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

Worth filing an issue against rustfmt about this. My personal experience is that in 99% of cases, programmers don't order their imports and it's not worth the extra effort for the other 1%. In particular I'm not sure if it helps anybody - I agree the enum ordering is sensible, but I don't see any concrete benefit.

eddyb added a commit to eddyb/rust that referenced this pull request Jun 6, 2016
@eddyb eddyb merged commit 8a6a9af into rust-lang:master Jun 6, 2016
@srinivasreddy srinivasreddy deleted the rustfmt_libtest branch June 6, 2016 08:26
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.

7 participants