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

[SR-146] swift-format tool #2197

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@johnno1962
Contributor

johnno1962 commented Apr 15, 2016

What's in this pull request?

An implementation of a swift-format tool to indent swift source code from the command line using the new libIDE api. Options based on "clang-format”. Performance is O(size of file * number of lines changed)

Resolved bug number:

https://bugs.swift.org/browse/SR-146

@johnno1962 johnno1962 changed the title from [SR-146swift-format tool to [SR-146] swift-format tool Apr 15, 2016

@nickoneill

This comment has been minimized.

Show comment
Hide comment
@nickoneill

nickoneill Apr 15, 2016

I would love to see more canonical (whatever that is) style enforcement come to Swift, this looks like a really great start. In particular, the location of colons 😂

Automatic code formatting to a single style has been in use with Go for a number of years and they're quite fond of it. I'd say the two biggest benefits are:

  • No arguments about style, ever
  • No extra effort getting used to someone's individual style when reading new code / PRs (particularly helpful for those new to Swift)

nickoneill commented Apr 15, 2016

I would love to see more canonical (whatever that is) style enforcement come to Swift, this looks like a really great start. In particular, the location of colons 😂

Automatic code formatting to a single style has been in use with Go for a number of years and they're quite fond of it. I'd say the two biggest benefits are:

  • No arguments about style, ever
  • No extra effort getting used to someone's individual style when reading new code / PRs (particularly helpful for those new to Swift)
@ejohnsonw

This comment has been minimized.

Show comment
Hide comment
@ejohnsonw

ejohnsonw Apr 15, 2016

This is great!

ejohnsonw commented Apr 15, 2016

This is great!

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Apr 15, 2016

Contributor

Extraneous spaces duly removed/added.

Contributor

johnno1962 commented Apr 15, 2016

Extraneous spaces duly removed/added.

@RLovelett

This comment has been minimized.

Show comment
Hide comment
@RLovelett

RLovelett Apr 18, 2016

Contributor

Squashing the commits with the exact same commit message will reduced entropy when reviewing.

Contributor

RLovelett commented Apr 18, 2016

Squashing the commits with the exact same commit message will reduced entropy when reviewing.

@mattyohe

This comment has been minimized.

Show comment
Hide comment
@mattyohe

mattyohe Apr 18, 2016

@RLovelett But wouldn't that break Github's recent additions that aid in tracking recent changes (assuming people use it ): https://github.com/blog/2123-more-code-review-tools

mattyohe commented Apr 18, 2016

@RLovelett But wouldn't that break Github's recent additions that aid in tracking recent changes (assuming people use it ): https://github.com/blog/2123-more-code-review-tools

@RLovelett

This comment has been minimized.

Show comment
Hide comment
@RLovelett

RLovelett Apr 18, 2016

Contributor

Maybe? Frankly I don't understand what is new about those features.

Regardless my biggest concern is 8 commits with the exact same message. No matter how these changes are reviewed that should be addressed. I merely suggested squashing to omit having to come up with 8 new commit messages. Any solution that rectifies the commit message issue is fine with me.

My review notes are thus amended to be:

Please have meaningful commit messages. Currently there are 8 commits with the exact same commit message. Additionally, please provide more information about the actual change. See the Swift Community notes for commit messages additionally the Git Book provides some helpful notes on writing good commit messages.

Contributor

RLovelett commented Apr 18, 2016

Maybe? Frankly I don't understand what is new about those features.

Regardless my biggest concern is 8 commits with the exact same message. No matter how these changes are reviewed that should be addressed. I merely suggested squashing to omit having to come up with 8 new commit messages. Any solution that rectifies the commit message issue is fine with me.

My review notes are thus amended to be:

Please have meaningful commit messages. Currently there are 8 commits with the exact same commit message. Additionally, please provide more information about the actual change. See the Swift Community notes for commit messages additionally the Git Book provides some helpful notes on writing good commit messages.

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Apr 18, 2016

Contributor

Hi, There are eight identical commit messages as they relate to essentially the same thing: whitespace formatting of swift-format.cpp, not functional changes. They're regrettable but I don’t think they make reviewing the changes more difficult in the finish as they all relate to a new file which must be reviewed as a whole anyway. I tried to squish them up but just ended up with two more commits!

I can restart this PR completely from a fresh fork to tidy it up if the Swift team shows any appetite for this imagining of swift-format functionality.

Contributor

johnno1962 commented Apr 18, 2016

Hi, There are eight identical commit messages as they relate to essentially the same thing: whitespace formatting of swift-format.cpp, not functional changes. They're regrettable but I don’t think they make reviewing the changes more difficult in the finish as they all relate to a new file which must be reviewed as a whole anyway. I tried to squish them up but just ended up with two more commits!

I can restart this PR completely from a fresh fork to tidy it up if the Swift team shows any appetite for this imagining of swift-format functionality.

@RLovelett

This comment has been minimized.

Show comment
Hide comment
@RLovelett

RLovelett Apr 18, 2016

Contributor

Obviously my comments will not ensure it gets merged or not. I am by no means going to be able to accept or reject this commit. All I am pointing out is a typical thing I've seen commented on other reviews that have held up reviewing and merging. I am trying to help because I think this is a value add tool to the standard toolset.

As for getting eyes on the changes. Perhaps a message to the mailing list? (Maybe you have, I don't mean to imply you have not)

Regarding the failed attempt at squishing. I might suggest reading Changing Multiple Commit Messages from the Git Book. Something like git rebase -i origin/master might just do the trick.

Contributor

RLovelett commented Apr 18, 2016

Obviously my comments will not ensure it gets merged or not. I am by no means going to be able to accept or reject this commit. All I am pointing out is a typical thing I've seen commented on other reviews that have held up reviewing and merging. I am trying to help because I think this is a value add tool to the standard toolset.

As for getting eyes on the changes. Perhaps a message to the mailing list? (Maybe you have, I don't mean to imply you have not)

Regarding the failed attempt at squishing. I might suggest reading Changing Multiple Commit Messages from the Git Book. Something like git rebase -i origin/master might just do the trick.

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Apr 19, 2016

Contributor

Thanks for the tip.. I gave it a try but I think a restart is probably going to be tidier if this PR is picked up.

Contributor

johnno1962 commented Apr 19, 2016

Thanks for the tip.. I gave it a try but I think a restart is probably going to be tidier if this PR is picked up.

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 19, 2016

Collaborator

@johnno1962 Try this:

git rebase -i HEAD~10 # you have 10 commits in this PR 
Change "pick" to "fixup" for all but the first line.
git push -f
Collaborator

practicalswift commented Apr 19, 2016

@johnno1962 Try this:

git rebase -i HEAD~10 # you have 10 commits in this PR 
Change "pick" to "fixup" for all but the first line.
git push -f
# This is a combination of 9 commits.
# The first commit's message is:
Add mangling for externally inlineable specializations

An upcoming change has the SIL Optimizer drop the [fragile]
attribute from the specialized callee, unless the caller
is itself [fragile].

Since we need to distinguish specializations from fragile
and non-fragile contexts, add a new mangling node to
represent this concept.

# The 2nd commit message will be skipped:

#	SILOptimizer: Create non-[fragile] specializations of [fragile] functions where possible
#
#	Change the optimizer to only make specializations [fragile] if both the
#	original callee is [fragile] *and* the caller is [fragile].
#
#	Otherwise, the specialized callee might be [fragile] even if it is never
#	called from a [fragile] function, which inhibits the optimizer from
#	devirtualizing calls inside the specialization.
#
#	This opens up some missed optimization opportunities in the performance
#	inliner and devirtualization, which currently reject fragile->non-fragile
#	references:
#
#	TEST                                                    | OLD_MIN | NEW_MIN | DELTA (%) | SPEEDUP
#	---                                                     | ---     | ---     | ---       | ---
#	DictionaryRemoveOfObjects                               | 38391   | 35859   | -6.6%     | **1.07x**
#	Hanoi                                                   | 5853    | 5288    | -9.7%     | **1.11x**
#	Phonebook                                               | 18287   | 14988   | -18.0%    | **1.22x**
#	SetExclusiveOr_OfObjects                                | 20001   | 15906   | -20.5%    | **1.26x**
#	SetUnion_OfObjects                                      | 16490   | 12370   | -25.0%    | **1.33x**
#
#	Right now, passes other than performance inlining and devirtualization
#	of class methods are not checking invariants on [fragile] functions
#	at all, which was incorrect; as part of the work on building the
#	standard library with -enable-resilience, I added these checks, which
#	regressed performance with resilience disabled. This patch makes up for
#	these regressions.
#
#	Furthermore, once SIL type lowering is aware of resilience, this will
#	allow the stack promotion pass to make further optimizations after
#	specializing [fragile] callees.

# The 3rd commit message will be skipped:

#	SIL: Better verifier check for references from fragile functions

# The 4th commit message will be skipped:

#	SILOptimizer: Fixes for non-fragile references in fragile functions
#
#	Two fixes to optimization passes to maintain restrictions about what
#	[fragile] functions can reference:
#
#	- When devirtualizing witness methods, don't devirtualize if the caller
#	  is fragile and the callee is not. This matches existing logic in
#	  class devirtualization.
#
#	- When performing generic or function signature specialization, don't
#	  specialize non-fragile functions referenced from fragile functions.
#
#	Since @_transparent functions are allowed to call 'static inline'
#	imported functions, also be sure to mark the foreign-to-native thunk
#	for such a function as [fragile].
#
#	With this patch, the standard library and performance test suite
#	now build with -enable-resilience.
#
#	No new tests for this stuff here -- the existing tests together
#	with an -enable-resilience build provide coverage.
#
#	Closes out <https://bugs.swift.org/browse/SR-267> and
#	<https://bugs.swift.org/browse/SR-268>.

# The 5th commit message will be skipped:

#	SIL: Stricter asserts for non-fragile references from fragile functions
#
#	Building off of the previous patches, add stricter assertions to
#	inlining passes and SIL serialization.

# The 6th commit message will be skipped:

#	SILGen: Explicitly make fragile entities public when -sil-serialize-all is on
#
#	Previously IRGen would force all fragile entities to have public linkage.
#	It makes more sense to do this in SILGen instead, and only when
#	-sil-serialize-all is on.
#
#	This patch was previously committed and reverted; the optimizer
#	issues exposed by the original version should now be fixed.

# The 7th commit message will be skipped:

#	IRGen: Remove special handling of fragile entities in LinkEntity
#
#	Dead code now that SILGen enforces this.
#
#	This patch was previously committed and reverted; the optimizer
#	issues exposed by the original version should now be fixed.

# The 8th commit message will be skipped:

#	swift-format tool

# The 9th commit message will be skipped:

#	swift-format tool formatting
@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Apr 19, 2016

Contributor

Do’h now the PR contains someone else’s commits and has conflicts.. I’ll start again from scratch.

Contributor

johnno1962 commented Apr 19, 2016

Do’h now the PR contains someone else’s commits and has conflicts.. I’ll start again from scratch.

@johnno1962 johnno1962 closed this Apr 19, 2016

@practicalswift

This comment has been minimized.

Show comment
Hide comment
@practicalswift

practicalswift Apr 19, 2016

Collaborator

Whoops, the N in HEAD~N part should have been the number of commits in your PR :-)

Collaborator

practicalswift commented Apr 19, 2016

Whoops, the N in HEAD~N part should have been the number of commits in your PR :-)

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Apr 20, 2016

Contributor

So close - and yet so far….

Contributor

johnno1962 commented Apr 20, 2016

So close - and yet so far….

@danielmartin

This comment has been minimized.

Show comment
Hide comment
@danielmartin

danielmartin Apr 29, 2016

Contributor

What's the status of this PR? Is it being reworked from scratch? I'm working in parallel on this issue with another goal in mind and I'd like to base on this PR, if possible.

Thank you.

Contributor

danielmartin commented Apr 29, 2016

What's the status of this PR? Is it being reworked from scratch? I'm working in parallel on this issue with another goal in mind and I'd like to base on this PR, if possible.

Thank you.

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 Apr 29, 2016

Contributor

Hi Daniel, This PR has met a sticky rebase and should not be used. You can take a clone of johnno1962e@13b5541 which I was preparing as a replacement if you want to pick up the torch.

Contributor

johnno1962 commented Apr 29, 2016

Hi Daniel, This PR has met a sticky rebase and should not be used. You can take a clone of johnno1962e@13b5541 which I was preparing as a replacement if you want to pick up the torch.

@RLovelett

This comment has been minimized.

Show comment
Hide comment
@RLovelett

RLovelett May 3, 2016

Contributor

@johnno1962 I've rebased johnno1962e@13b5541 on master (d2aee43) and made RLovelett@44ef116.

I would like to see you get this merged. Please let me know what I can do to help you @johnno1962.

Contributor

RLovelett commented May 3, 2016

@johnno1962 I've rebased johnno1962e@13b5541 on master (d2aee43) and made RLovelett@44ef116.

I would like to see you get this merged. Please let me know what I can do to help you @johnno1962.

@johnno1962

This comment has been minimized.

Show comment
Hide comment
@johnno1962

johnno1962 May 3, 2016

Contributor

Hi Ryan, feel free to submit your rebase as a PR to swift. I’d like to see this merged too but I’m unsure if there is any takeup from Apple.

Contributor

johnno1962 commented May 3, 2016

Hi Ryan, feel free to submit your rebase as a PR to swift. I’d like to see this merged too but I’m unsure if there is any takeup from Apple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment