Skip to content

rustfmt librand/distributions #29054

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

Closed
wants to merge 8 commits into from
Closed

rustfmt librand/distributions #29054

wants to merge 8 commits into from

Conversation

mseri
Copy link
Contributor

@mseri mseri commented Oct 14, 2015

r? @nrc

I was wondering, what is the standard for pattern matching in presence of guards? I liked more the original alignment of distributions/gamma.rs line 91 than the new one.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nrc
Copy link
Member

nrc commented Oct 14, 2015

@mseri in general, rustfmt does not try to align tokens between sequential items, for example the => in the match arms here. Sometimes it is desirable, but it is really hard for rustfmt to tell the difference between intentional alignment and whitespace errors. For now, the policy is to accept this and where something looks much better with alignment, do it manually and add skip attributes. I think in this case rustfmt's version is OK.

@nrc
Copy link
Member

nrc commented Oct 14, 2015

@bors: r+ rollup

Thanks for the PR, this looks great!

@bors
Copy link
Collaborator

bors commented Oct 14, 2015

📌 Commit 84b77f1 has been approved by nrc

@mseri
Copy link
Contributor Author

mseri commented Oct 14, 2015

Ok. Thanks for the explanations. Should I submit a PR to the style guide adding some of the missing style guidelines that I saw while doing these rustfmt (and the ones in preparation)?

@nrc
Copy link
Member

nrc commented Oct 15, 2015

You can if you like, the style guide is lacking a lot of the detail of rustfmt. I expect we'll revamp it properly at some point in the future

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Oct 15, 2015
r? @nrc

I was wondering, what is the standard for pattern matching in presence of guards? I liked more the original alignment of `distributions/gamma.rs` line 91 than the new one.
@@ -103,7 +107,8 @@ impl<'a, T: Clone> WeightedChoice<'a, T> {
/// - the total weight is larger than a `usize` can contain.
pub fn new(items: &'a mut [Weighted<T>]) -> WeightedChoice<'a, T> {
// strictly speaking, this is subsumed by the total weight == 0 case
assert!(!items.is_empty(), "WeightedChoice::new called with no items");
assert!(!items.is_empty(),
Copy link
Member

Choose a reason for hiding this comment

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

This getting split into two lines makes little sense @nrc.

Copy link
Member

Choose a reason for hiding this comment

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

We split when the internals of a function call or function-like macro is more than a heuristic limit (I think 60 chars). Mostly this works well. Occasionally (like here) it seems a bit over the top. I expect its a heuristic we'll polish as time goes by.

@mseri
Copy link
Contributor Author

mseri commented Oct 15, 2015

@steveklabnik there is a typo that I fixed this morning. My apologies for that, I ran the tests on the wrong patch branch.

Additionally, there is this:

src/librand/distributions/ziggurat_tables.rs:16:1: 16:16 error: The attribute `rustfmt_skip` is currently unknown to the compiler and may have meaning added to it in the future
src/librand/distributions/ziggurat_tables.rs:16 #[rustfmt_skip]
                                                ^~~~~~~~~~~~~~~
src/librand/distributions/ziggurat_tables.rs:16:1: 16:16 help: add #![feature(custom_attribute)] to the crate attributes to enable
src/librand/distributions/ziggurat_tables.rs:83:1: 83:16 error: The attribute `rustfmt_skip` is currently unknown to the compiler and may have meaning added to it in the future
src/librand/distributions/ziggurat_tables.rs:83 #[rustfmt_skip]
                                                ^~~~~~~~~~~~~~~
src/librand/distributions/ziggurat_tables.rs:83:1: 83:16 help: add #![feature(custom_attribute)] to the crate attributes to enable
src/librand/distributions/ziggurat_tables.rs:151:1: 151:16 error: The attribute `rustfmt_skip` is currently unknown to the compiler and may have meaning added to it in the future
src/librand/distributions/ziggurat_tables.rs:151 #[rustfmt_skip]
                                                 ^~~~~~~~~~~~~~~
src/librand/distributions/ziggurat_tables.rs:151:1: 151:16 help: add #![feature(custom_attribute)] to the crate attributes to enable
src/librand/distributions/ziggurat_tables.rs:218:1: 218:16 error: The attribute `rustfmt_skip` is currently unknown to the compiler and may have meaning added to it in the future
src/librand/distributions/ziggurat_tables.rs:218 #[rustfmt_skip]

@nrc should I add the feature attrubute or remove the skips? For the vectors refactored there rustfmt should not be executed, or one ends up with an immensely long unreadable file.

@mseri
Copy link
Contributor Author

mseri commented Oct 15, 2015

I think we should just get rid of those rustfmt_skip, is there any other way we can tell rustfmt to skip the file?

@nrc
Copy link
Member

nrc commented Oct 15, 2015

You need to add #![feature(custom_attribute)] and #![allow(unused_attributes)] to the crate root to pass tests.

The skip attribute is the only way to get rustfmt to skip a file or item.

@mseri
Copy link
Contributor Author

mseri commented Oct 16, 2015

Sorry, I messed up with this branch. I am closing the pull request - cleaning everything and resubmitting it properly.

@mseri mseri closed this Oct 16, 2015
@mseri mseri deleted the patch-5 branch October 16, 2015 20:56
@mseri mseri restored the patch-5 branch October 16, 2015 20:58
@mseri mseri deleted the patch-5 branch October 16, 2015 20:58
@mseri mseri mentioned this pull request Oct 17, 2015
bors added a commit that referenced this pull request Oct 18, 2015
r? @nrc

Re-submission of the closed PR #29054 with the additional rustfmt-zation of the full librand.
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.

5 participants