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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/libtest/stats.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

use std::mem;

fn local_cmp(x: f64, y: f64) -> Ordering {
Expand All @@ -35,7 +35,6 @@ fn local_sort(v: &mut [f64]) {

/// Trait that provides simple descriptive statistics on a univariate set of numeric samples.
pub trait Stats {

/// Sum of the samples.
///
/// Note: this method sacrifices performance at the altar of accuracy
Expand Down