Run rustfmt on liballoc #28610

Merged
merged 3 commits into from Sep 25, 2015

Conversation

Projects
None yet
8 participants
@nrc
Member

nrc commented Sep 23, 2015

No description provided.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Sep 23, 2015

Collaborator

r? @nikomatsakis

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

Collaborator

rust-highfive commented Sep 23, 2015

r? @nikomatsakis

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

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Sep 23, 2015

Member

r? @brson

Member

nrc commented Sep 23, 2015

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Sep 23, 2015

@@ -251,7 +253,9 @@ impl<T: ?Sized> Arc<T> {
let cur = this.inner().weak.load(Relaxed);
// check if the weak counter is currently "locked"; if so, spin.
- if cur == usize::MAX { continue }
+ if cur == usize::MAX {
+ continue

This comment has been minimized.

@steveklabnik

steveklabnik Sep 23, 2015

Member

since this is a style PR, I wonder: is it good style to use ; here? I probably would. I know it's not in the original source, so it's not rustfmt doing it.

@steveklabnik

steveklabnik Sep 23, 2015

Member

since this is a style PR, I wonder: is it good style to use ; here? I probably would. I know it's not in the original source, so it's not rustfmt doing it.

This comment has been minimized.

This comment has been minimized.

@alexcrichton

alexcrichton Sep 24, 2015

Member

I personally prefer to leave off ; whenever it's a terminating statement that can't have anything following it anyway (e.g. break, return, continue) but that may be just my style

@alexcrichton

alexcrichton Sep 24, 2015

Member

I personally prefer to leave off ; whenever it's a terminating statement that can't have anything following it anyway (e.g. break, return, continue) but that may be just my style

@@ -556,7 +562,9 @@ impl<T: ?Sized> Drop for Arc<T> {
// Because `fetch_sub` is already atomic, we do not need to synchronize
// with other threads unless we are going to delete the object. This
// same logic applies to the below `fetch_sub` to the `weak` count.
- if self.inner().strong.fetch_sub(1, Release) != 1 { return }
+ if self.inner().strong.fetch_sub(1, Release) != 1 {
+ return

This comment has been minimized.

@steveklabnik

steveklabnik Sep 23, 2015

Member

same here

@steveklabnik

steveklabnik Sep 23, 2015

Member

same here

@@ -348,7 +352,9 @@ impl<T: ?Sized> Clone for Arc<T> {
// We abort because such a program is incredibly degenerate, and we
// don't care to support it.
if old_size > MAX_REFCOUNT {
- unsafe { abort(); }
+ unsafe {
+ abort();

This comment has been minimized.

@steveklabnik

steveklabnik Sep 23, 2015

Member

... like, one did get used here

@steveklabnik

steveklabnik Sep 23, 2015

Member

... like, one did get used here

@@ -276,8 +284,11 @@ impl<T: Clone> Clone for Box<T> {
/// let x = Box::new(5);
/// let y = x.clone();
/// ```
+ #[rustfmt_skip]

This comment has been minimized.

@steveklabnik

steveklabnik Sep 23, 2015

Member

this was added, yet it still was reformatted?

@steveklabnik

steveklabnik Sep 23, 2015

Member

this was added, yet it still was reformatted?

This comment has been minimized.

@nrc

nrc Sep 23, 2015

Member

No, it was reformatted, then I manually un-re-formatted part of it and added the attribute

@nrc

nrc Sep 23, 2015

Member

No, it was reformatted, then I manually un-re-formatted part of it and added the attribute

This comment has been minimized.

@steveklabnik

steveklabnik Sep 23, 2015

Member

ah! 👍

- data: RawVec::with_capacity(self.len()),
- len: 0
- };
+ let mut new = BoxBuilder { data: RawVec::with_capacity(self.len()), len: 0 };

This comment has been minimized.

@nrc

nrc Sep 23, 2015

Member

This is perhaps a little sub-optimal, but I can live with it (see https://github.com/nrc/rustfmt/issues/123)

@nrc

nrc Sep 23, 2015

Member

This is perhaps a little sub-optimal, but I can live with it (see https://github.com/nrc/rustfmt/issues/123)

This comment has been minimized.

@steveklabnik

steveklabnik Sep 23, 2015

Member

ah! was just wondering about this, too. I think it looks fine in this case, either way, though I'd probably write the before myself

@steveklabnik

steveklabnik Sep 23, 2015

Member

ah! was just wondering about this, too. I think it looks fine in this case, either way, though I'd probably write the before myself

@@ -70,7 +74,8 @@ fn test_show() {
#[test]
fn deref() {
- fn homura<T: Deref<Target=i32>>(_: T) { }
+ fn homura<T: Deref<Target = i32>>(_: T) {
+ }

This comment has been minimized.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Sep 23, 2015

Member

The bits Steve and I identified as sub-optimal are all bits which will be changed if we re-run with an improved rustfmt, so I don't think they should block landing this PR.

Member

nrc commented Sep 23, 2015

The bits Steve and I identified as sub-optimal are all bits which will be changed if we re-run with an improved rustfmt, so I don't think they should block landing this PR.

src/liballoc/arc.rs
- unsafe {
- self.drop_slow()
- }
+ unsafe { self.drop_slow() }

This comment has been minimized.

@Gankro

Gankro Sep 23, 2015

Contributor

Why is this one turned into one-line, but the previous unsafe block has the precise opposite?

@Gankro

Gankro Sep 23, 2015

Contributor

Why is this one turned into one-line, but the previous unsafe block has the precise opposite?

This comment has been minimized.

@nrc

nrc Sep 23, 2015

Member

We allow one-line unsafe blocks if there is a single expression, the previous one is a single statement. This is obviously an imperfect heuristic, foiled here because there is no semi-colon, even though we don't use the value. We don't have type info in rustfmt (for now), so can't insert the semicolon. I'll fix this instance manually.

@nrc

nrc Sep 23, 2015

Member

We allow one-line unsafe blocks if there is a single expression, the previous one is a single statement. This is obviously an imperfect heuristic, foiled here because there is no semi-colon, even though we don't use the value. We don't have type info in rustfmt (for now), so can't insert the semicolon. I'll fix this instance manually.

+ !0
+ } else {
+ self.cap
+ }

This comment has been minimized.

@Gankro

Gankro Sep 23, 2015

Contributor

Do we just totally reject ternaries?

@Gankro

Gankro Sep 23, 2015

Contributor

Do we just totally reject ternaries?

This comment has been minimized.

@nrc

nrc Sep 23, 2015

Member

yes

@nrc

nrc Sep 23, 2015

Member

yes

src/liballoc/raw_vec.rs
- self.cap * elem_size,
- new_alloc_size,
- align)
+ heap::reallocate(self.ptr() as *mut _, self.cap * elem_size, new_alloc_size, align)

This comment has been minimized.

@Gankro

Gankro Sep 23, 2015

Contributor

This is pretty unfortunate IMO, but perhaps a case of semantic info being superior?

@Gankro

Gankro Sep 23, 2015

Contributor

This is pretty unfortunate IMO, but perhaps a case of semantic info being superior?

This comment has been minimized.

@Gankro

Gankro Sep 23, 2015

Contributor

Wonder if there's a heuristic involving argument complexity?

@Gankro

Gankro Sep 23, 2015

Contributor

Wonder if there's a heuristic involving argument complexity?

This comment has been minimized.

@nrc

nrc Sep 23, 2015

Member

See https://github.com/nrc/rustfmt/issues/342

I'm actually working on this right now :-)

@nrc

nrc Sep 23, 2015

Member

See https://github.com/nrc/rustfmt/issues/342

I'm actually working on this right now :-)

nrc added some commits Sep 23, 2015

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Sep 23, 2015

Member

@Gankro better?

Member

nrc commented Sep 23, 2015

@Gankro better?

@Gankro

This comment has been minimized.

Show comment
Hide comment
@Gankro

Gankro Sep 24, 2015

Contributor

I'm pretty bumbed out about losing all these sweet one-liners (especially the ternaries and trait impls)...

But the most egregious affronts to the unsafe gods have been removed, for sure.

Contributor

Gankro commented Sep 24, 2015

I'm pretty bumbed out about losing all these sweet one-liners (especially the ternaries and trait impls)...

But the most egregious affronts to the unsafe gods have been removed, for sure.

@brson

This comment has been minimized.

Show comment
Hide comment
@brson

brson Sep 24, 2015

Contributor

r=me when you are satisfied comments addressed

Contributor

brson commented Sep 24, 2015

r=me when you are satisfied comments addressed

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Sep 24, 2015

Member

I think all the comments (except Gankro's which I don't think can be easily addressed) are about possible improvements rather than things that need to be done now.

@bors: r=brson

Member

nrc commented Sep 24, 2015

I think all the comments (except Gankro's which I don't think can be easily addressed) are about possible improvements rather than things that need to be done now.

@bors: r=brson

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 24, 2015

Contributor

📌 Commit 459f772 has been approved by brson

Contributor

bors commented Sep 24, 2015

📌 Commit 459f772 has been approved by brson

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 24, 2015

Contributor

⌛️ Testing commit 459f772 with merge 0683d36...

Contributor

bors commented Sep 24, 2015

⌛️ Testing commit 459f772 with merge 0683d36...

bors added a commit that referenced this pull request Sep 24, 2015

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 24, 2015

Contributor

💔 Test failed - auto-linux-32-opt

Contributor

bors commented Sep 24, 2015

💔 Test failed - auto-linux-32-opt

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 24, 2015

Member

@bors: retry

On Thu, Sep 24, 2015 at 4:32 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/6560


Reply to this email directly or view it on GitHub
#28610 (comment).

Member

alexcrichton commented Sep 24, 2015

@bors: retry

On Thu, Sep 24, 2015 at 4:32 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/6560


Reply to this email directly or view it on GitHub
#28610 (comment).

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Sep 25, 2015

Contributor

⌛️ Testing commit 459f772 with merge e7a7388...

Contributor

bors commented Sep 25, 2015

⌛️ Testing commit 459f772 with merge e7a7388...

bors added a commit that referenced this pull request Sep 25, 2015

@bors bors merged commit 459f772 into rust-lang:master Sep 25, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment