Skip to content

Rustfmt-ing librustc_front. - #29075

Closed
goyox86 wants to merge 4 commits into
rust-lang:masterfrom
goyox86:goyox86/rustfmting-librustc_front
Closed

Rustfmt-ing librustc_front.#29075
goyox86 wants to merge 4 commits into
rust-lang:masterfrom
goyox86:goyox86/rustfmting-librustc_front

Conversation

@goyox86

@goyox86goyox86 commented Oct 15, 2015

Copy link
Copy Markdown
Contributor

Hi Rustaceans!

This is the result of running latest rustfmt on librustc_front!

//cc @nrc

@rust-highfive

rust-highfive commented Oct 15, 2015

Copy link
Copy Markdown
Contributor

r? @alexcrichton

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

petrochenkov
petrochenkov reviewed Oct 15, 2015
View reviewed changes
Comment threadsrc/librustc_front/fold.rs

@petrochenkovpetrochenkovOct 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

BTW, I dislike the current formatting of struct literals/struct patterns. I'd even say it discourages their use somewhat.
Compare struct patterns and tuple patterns:

S { name, attrs, data, disr_expr } // those damn 3 extra spaces! I need to type them (okay, rustfmt solve this problem) and they shift the text closer to 100 character limit
S { .. } // even worse, 37.5% of unnecessary symbols
S(name, attrs, data, disr_expr) // nice
S(..) // nice
S{name, attrs, data, disr_expr} // almost as nice
S{..} // almost as nice

@alexcrichton

alexcrichton commented Oct 15, 2015

Copy link
Copy Markdown
Member

r? @nrc

nrc
nrc reviewed Oct 15, 2015
View reviewed changes
Comment threadsrc/librustc_front/hir.rs Outdated

@nrcnrcOct 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We lose this comment, I imagine it is because the empty line comment is a doc comment, it should probably lose a /, then hopefully rustfmt won't screw this up. Needs a fixup.

@nrcnrcOct 21, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This still needs fixing up

@nrc

nrc commented Oct 15, 2015

Copy link
Copy Markdown
Member

Just needs one fixup, r+ with that

cristicbz
cristicbz reviewed Oct 16, 2015
View reviewed changes
Comment threadsrc/librustc_front/hir.rs

@cristicbzcristicbzOct 16, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure the above is worse than original.

@nrcnrcOct 18, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

rustfmt fix is in the works...

@nrcnrcOct 21, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/nrc/rustfmt/pull/485

@nrcnrcOct 26, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be fixed now

@bors

bors commented Oct 17, 2015

Copy link
Copy Markdown
Collaborator

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

@goyox86
goyox86force-pushed the goyox86/rustfmting-librustc_front branch from 5dfa87e to a09d94cCompareOctober 21, 2015 13:38
@goyox86

goyox86 commented Oct 21, 2015

Copy link
Copy Markdown
ContributorAuthor

@nrc re-ran rustfmt again. Also added a manual fixup.

nrc
nrc reviewed Oct 21, 2015
View reviewed changes
Comment threadsrc/librustc_front/lowering.rs

@nrcnrcOct 21, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/nrc/rustfmt/issues/508

This is causing build to fail

@nrcnrcOct 26, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should also be fixed

@nrc

nrc commented Oct 21, 2015

Copy link
Copy Markdown
Member

Running rustfmt again undid your fixup. I think rather than just reinstating the deleted comment, you need to change the blank line comment from /// to //, then rustfmt should not delete the comment again.

Annoyingly rustfmt has regressed some how and is now deleting a comma.

@bors

bors commented Oct 26, 2015

Copy link
Copy Markdown
Collaborator

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

nrc
nrc reviewed Oct 26, 2015
View reviewed changes
Comment threadsrc/librustc_front/hir.rs

@nrcnrcOct 26, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/nrc/rustfmt/issues/539

Could you fix this manually for now please?

@goyox86goyox86 mentioned this pull request Nov 5, 2015
Merged
@goyox86

goyox86 commented Nov 5, 2015

Copy link
Copy Markdown
ContributorAuthor

Closing this on favor of #29647

@goyox86goyox86 closed this Nov 5, 2015
bors added a commit that referenced this pull request Nov 9, 2015
@bors
Auto merge of#29647- goyox86:goyox86/rustfmting-librustc_frontII, r…
205406c
…=nrc
Hi Rustaceans!
This is the second take on running latest rustfmt on librustc_front!
This is the same in #29075 but cleaned. All fixups have been applied.
//cc @nrc
bors added a commit that referenced this pull request Nov 10, 2015
@bors
Auto merge of#29647- goyox86:goyox86/rustfmting-librustc_frontII, r…
4b23a09
…=nrc
Hi Rustaceans!
This is the second take on running latest rustfmt on librustc_front!
This is the same in #29075 but cleaned. All fixups have been applied.
//cc @nrc
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nrcnrc

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@goyox86@rust-highfive@alexcrichton@nrc@bors@cristicbz@petrochenkov