Skip to content
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

run rustfmt on libcollections/btree folder #34087

Closed
wants to merge 1 commit into from
Closed

run rustfmt on libcollections/btree folder #34087

wants to merge 1 commit into from

Conversation

srinivasreddy
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @brson

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

unsafe {
BoxedNode { ptr: Unique::new(Box::into_raw(node)) }
}
unsafe { BoxedNode { ptr: Unique::new(Box::into_raw(node)) } }
Copy link
Member

Choose a reason for hiding this comment

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

The previous version was much better here. Ditto below.

@arielb1
Copy link
Contributor

arielb1 commented Jun 5, 2016

cc @nrc

unsafe impl<K: Sync, V: Sync> Sync for Root<K, V> { }
unsafe impl<K: Send, V: Send> Send for Root<K, V> { }
unsafe impl<K: Sync, V: Sync> Sync for Root<K, V> {}
unsafe impl<K: Send, V: Send> Send for Root<K, V> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seriously surprises me - every empty brace pair I've seen in the Rust codebase has had a space in the middle.

@gereeter
Copy link
Contributor

gereeter commented Jun 5, 2016

There are a few things shown in this PR that I just don't really understand:

  • rustfmt seem to universally favor multiline methods, splitting the brackets onto separate lines even when the method body is short. However, it collapses small enough unsafe blocks into a single line. However, it splits struct initializers onto multiple lines, even when their body is short. It just seems inconsistent to me.
  • rustfmt likes the spaces after { and before } when there is actually code between the brackets, but goes for {} instead of { } when there is none. This seems weird, primarily because I've seen { } all over the Rust codebase - it seems to be accepted style.

I don't want this PR to devolve into bickering about meaningless style, and there definitely are many clear improvements here, but those stuck out to me as odd and, more importantly, divergent from what the rest of the Rust codebase has accepted.

As a side note, since most of what is being edited here is my code, this PR paints a really fascinating, clear picture of where my personal style diverges from rustfmt's.

let mut new_root = Root {
node: edge,
height: internal.height - 1,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine (in fact, I like the expanded look a bit better), but it surprises me given that rustfmt kept some longer and more cumbersome struct initializers as a single line farther up.

@gereeter
Copy link
Contributor

gereeter commented Jun 5, 2016

Thank you for doing this - when I wrote this code, I remember explicitly making completely arbitrary style choices multiple times, since nothing I thought of looked particularly good. I will admit that this module is somewhat of a worst case for rustfmt or any human trying to find a good style, with function signatures containing excessively large and deeply nested types, often with some where clauses or just bounds in the generics, all while being already at a reasonably deep indentation level. There are definitely places where attempting to split a line while aligning things nicely doesn't actually prevent overflowing the line.

@srinivasreddy
Copy link
Contributor Author

@nrc Please respond to @gereeter comments. This PR looks like all hell broke loose. 😄

@gereeter
Copy link
Contributor

gereeter commented Jun 6, 2016

Also, it definitely seems like a bug that rustfmt generated overlong lines.

@nrc
Copy link
Member

nrc commented Jun 7, 2016

There's a few places here which could be improved, but I don't think there are any where I would get Rustfmt to skip. In general, I'm happy to eat a few spots of sub-optimal formatting in order to get automated formatting everywhere. We could always add rustfmt_skip if there are places that @gereeter feels are obnoxiously bad.

Overlong lines are bugs we should fix (before landing).

@nrc
Copy link
Member

nrc commented Jun 7, 2016

I filed rust-lang/rustfmt#1048 and rust-lang/rustfmt#1049 for the overlong lines

@bors
Copy link
Contributor

bors commented Jun 11, 2016

☔ The latest upstream changes (presumably #34153) made this pull request unmergeable. Please resolve the merge conflicts.

@brson
Copy link
Contributor

brson commented Jul 11, 2016

I'm still not sure what to do here. The code does look quite bad in places, and to bring this patch up to date rustfmt will need to be rerun completely.

@nrc
Copy link
Member

nrc commented Jul 15, 2016

let's close for now, we can open a new PR once those bugs get fixed in Rustfmt

@nrc nrc closed this Jul 15, 2016
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.

8 participants