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

Format 3.10 #4349

Open
wants to merge 11 commits into
base: 3.10
from

Conversation

Projects
None yet
@brianlheim
Copy link
Member

brianlheim commented Mar 5, 2019

DO NOT MERGE.

I'm going to squash all the "reformat:" commits after giving them a quick sanity check. Would really appreciate if someone else can also look over the changes to make sure there isn't anything obviously wrong.

Here are some notes I took on the process, which I'm going to use to make the develop version of this PR tomorrow.

A. formatting C++ files
1. add .clang-format file
2. to find all relevant source files: find . | grep -E '.*\.(cpp|hpp|h|c|cc|hh|cxx|hxx|m|mm)$' | sed 's:^\./([^/]*)/.*:\1:' | sort | uniq
3. to format those files: find QtCollider SCDoc common editors include lang platform server testsuite tools package -type f | grep -E '^[^\.]*\.(cpp|hpp|h|c|cc|hh|cxx|hxx|m|mm)$' | xargs clang-format -i
4. ran 3 times until no more changes (clang-format has (or had) a bug where it reflowed comments in a non idempotent way)

B. formatting cmake
1. to find all CMake files, ignoring those in external_libraries subdirectories and in scvim submodule: find . -name 'CMakeLists.txt' | grep -v 'external_libraries/.*/\|scvim'
2. command to format CMake files: for file in $(find . -name 'CMakeLists.txt' | grep -v 'external_libraries/.*/\|scvim'); do vim $file -c ':set et' -c ':normal =G' -c ':%s/^\(\s*\w*\) (/\1(/e' -c ':%s/^\(\s*[A-Z_]\+\)(/\L\1\E(/e' -c ':%s/else(.\+)/else()/e' -c ':%s/end\(\w\+\)(.\+)/end\1()/e' -c ':%s/^\(\s*\w\+\)( /\1(/e' -c ':%s/^\(\s*\w\+(.*\) )$/\1)/e' -c ':wq'; done
3. to confirm it worked: find . -name 'CMakeLists.txt' | grep -v 'external_libraries/.*/\|scvim' | xargs cmakelint --filter=-linelength 2>/dev/null
4. hand-edit for remaining failures (a couple tricky whitespace usage issues)

C. formatting python
1. discovering python files: find tools testsuite editors package -name '*.py'
2. formatting, remove trailing ws: for file in $(find tools testsuite editors package -name '*.py'); do vim $file -c ':%s/ *$//e' -c ':wq'; done

D. formatting schelp
1. formatting: removing trailing ws. need to be careful for octal 240 and decimal 8232 (non-breaking whitespace characters): for file in $(find HelpSource -name '*.schelp' -exec grep -l "\s\+$" {} \;); do vim $file -c ':%s/\(\s\|\%o240\|\%d8232\)\+$' -c ':wq'; done
2. validating: find HelpSource -name '*.schelp' -exec grep -nH "\s\+$" {} \;

E. formatting sc/scd files
1. rename all files with spaces in names:
cd examples
mv GUI\ examples/ GUI_examples
find . -type f -name '* *' -print0 | while IFS= read -r -d '' file; do mv "$file" $(echo "$file" | sed 's/ /_/g'); done
cd ..
2. remove trailing ws:
for file in $(find platform icons examples testsuite SCClassLibrary -name '*.sc*' -exec grep -l "\s\+$" {} \; | grep -v '\.scsyndef'); do vim $file -c ':%s/\r/\r/ge' -c ':%s/\s\+$//e' -c ':wq'; done
4. to validate: find platform icons examples testsuite SCClassLibrary -name '*.sc?' -exec grep -nH "\s\+$" {} \;

5. changing leading space (hand-edit required):
for file in $(find SCClassLibrary testsuite examples -name '*.sc*' -exec grep -l "^\t* \t*\([^*]\|$\)" {} \; | grep -v 'scsyndef'); do vim $file -c '+/^\t* \t*\([^*]\|$\)'; done

technique:
1. simply rely on scvim indenting where possible
2. for multiline comments, using " *" prefix

validating:
grep -nHrI "\s\+$" SCClassLibrary testsuite examples # trailing
find SCClassLibrary testsuite examples -name '*.sc*' -exec grep -nH "^\t* \t*\([^*]\|$\)" {} \; # leading

F. various small files
finding:
grep -nHrI "\s\+$" HelpSource
vim $(grep -lrI "\s\+$" SCClassLibrary common editors examples icons include lang package platform server sounds testsuite tools)

hand-edited most of them due to space in filename (Standalone Resources)

validating: grep -lrI "\s\+$" SCClassLibrary common editors examples icons include lang package platform server sounds testsuite tools | grep -v 'editors/scvim'

G. sanity check: lint-all.sh script
tools/dev/lint-all.sh
// Uncomment this if you want to see the current r value
//("// "++ r).postln;
60.do({x = r * x * (1.0 - x); x.yield;}); });
// Initial value

This comment has been minimized.

@snappizz

snappizz Mar 5, 2019

Member

quick skim -- this indentation looks incorrect.

@brianlheim brianlheim marked this pull request as ready for review Mar 5, 2019

@brianlheim brianlheim closed this Mar 5, 2019

@brianlheim brianlheim reopened this Mar 5, 2019

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 5, 2019

checking Travis CI...

@geoffroymontel

This comment has been minimized.

Copy link
Contributor

geoffroymontel commented Mar 5, 2019

Thanks for doing that !!

@@ -468,7 +468,7 @@ PmonoArtic(
)


//richer sequence with some heavier moments
//richer sequence with some heavier moments

This comment has been minimized.

@mtmccrea

mtmccrea Mar 5, 2019

Contributor

In your note on technique you mentioned:

  1. for multiline comments, using "// " prefix instead

So should lines with //xxx be changed to // xxx? (space added after //)

This comment has been minimized.

@mtmccrea

mtmccrea Mar 5, 2019

Contributor

I suppose the question stands for single-line comments as well.

*
* (andrea valle)
*/
* Two Auditory Scene Analysis (ASA) experiences

This comment has been minimized.

@mtmccrea

mtmccrea Mar 5, 2019

Contributor

Looks like an edge case multi-line comment, there are a couple more of these. Not sure if you want to catch them?

* added to the start of an error-causing string to make sure
* it causes an error.
*/
* Will be added to the end of a test comment to make sure it

This comment has been minimized.

@mtmccrea

mtmccrea Mar 5, 2019

Contributor

another example of edge case multi-line comment

@gusano

This comment has been minimized.

Copy link
Member

gusano commented Mar 5, 2019

@brianlheim thanks so much for doing that!
A small note on reviewing.. It might have been easier to do several PRs, maybe one per file type? (feel free to ignore, this is only a note :)
Also it seems you are committing changes to scvim submodule.. not sure that was intended..
But otherwise merci!

+ Prand([0, 0, 0, [0, 3], [0, 1]], inf),
\dur, Pseq([1, 2, 0.3, 0.5, 0.5], inf) + (Prand([0.3, 0, 0.2], inf) * 0.1),
\detune, Pseg(Pwhite(-2, 2, inf), 1.3)
)
).play
);
)

This comment has been minimized.

@mtmccrea

mtmccrea Mar 5, 2019

Contributor

Elsewhere in this file, you'll find indentation that is idiomatic to the author, but departs from SC's convention, see these lines for example.
Do you want to catch this kind of thing in this reformatting?

parseTechnique
************************************/
// ****** LAYOUT OF THIS FILE *******

This comment has been minimized.

@telephon

telephon Mar 5, 2019

Member

out of curiosity: what is the reason for this conversion? I always found it much easier to deal with /* – */ than with multiline //, and it looks better to me.

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 6, 2019

Thanks everyone for your comments, I'm glad it's getting thorough review because I am hesitant to merge something like this on my own.

@gusano

A small note on reviewing.. It might have been easier to do several PRs, maybe one per file type? (feel free to ignore, this is only a note :)

I am going to squash it all, reasoning is best explained here: #2819 (comment)

Also it seems you are committing changes to scvim submodule.. not sure that was intended..

It was indeed not intended, thanks for catching that!

@mtmccrea

So should lines with //xxx be changed to // xxx? (space added after //)

I avoided anything beyond what I thought was the bare minimum for linting, which would be:

  • no trailing whitespace
  • no mixed leading whitespace (i.e. no leading spaces)

More sophisticated linting/formatting is the domain of a syntax-aware linter, which we do not have at the moment, and which I didn't want to try to hack together out of grep/sed commands :)

Elsewhere in this file, you'll find indentation that is idiomatic to the author, but departs from SC's convention, see these lines for example.
Do you want to catch this kind of thing in this reformatting?

No, for the same reason; there is no reasonable place to draw the line once you start considering changes like this, so I am only going with what the community has already agreed on: that SC code should not be indented with spaces.

@mtmccrea

Looks like an edge case multi-line comment, there are a couple more of these. Not sure if you want to catch them?

@telephon

out of curiosity: what is the reason for this conversion? I always found it much easier to deal with /* – */ than with multiline //, and it looks better to me.

Both of these comments are about the same conversion, so I am responding to them at the same time.

I was not totally sure how to convert these. The one thing I did want was for there to be no leading spaces, ever, in an SC file, which seems consistent with what we've agreed upon. Right now, the most reasonably sophisticated thing I can think to do is lint by grepping ^\s* + (i.e., lines that contain leading whitespace, at least one character of which is a space). So, multiline comments that are formatted in either of these two styles won't work:

/* line 1
 * line 2
 */

/* line 1
   line 2
   line 3
*/

I don't know what the preferable way would be of formatting multiline comments under these restrictions. I came up with these two:

/* line 1
* line 2
*/

// line 1
// line 2
// line 3

another option would be

/*
line 1
line 2
*/

but this is incompatible with ASCII art.

How about for now, we just format them as

/* line 1
 * line 2
 * line 3
 */

and I write the linting pattern to pass lines that match ^ *? if it ever comes up again we can decide what to do about it then; this is a handful of changes.

@mtmccrea

This comment has been minimized.

Copy link
Contributor

mtmccrea commented Mar 6, 2019

Thanks for following up, Brian.

I avoided anything beyond what I thought was the bare minimum for linting...

there is no reasonable place to draw the line once you start considering changes like this, so I am only going with what the community has already agreed on

Your rationale for both of these cases seems reasonable to me. 👍

How about for now, we just format them as

/* line 1
 * line 2
 * line 3
 */

This seems like the cleanest of your suggested approaches, though I don't personally have a strong feeling about it.

Thanks again, Brian!

@brianlheim brianlheim force-pushed the brianlheim:format-3.10 branch from 762a632 to 313e4ba Mar 7, 2019

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 7, 2019

This seems like the cleanest of your suggested approaches, though I don't personally have a strong feeling about it.

Ok, I've done that. It actually led to catching a few more cases of incorrect mixed whitespace.

Ready for another review. I'm going to continue with creating the develop PR.

@brianlheim brianlheim referenced this pull request Mar 7, 2019

Open

Format develop #4350

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 7, 2019

Also it seems you are committing changes to scvim submodule.. not sure that was intended..

@gusano While committing C++ changes I accidentally committed the latest version of that submodule, which I had checked out. That did not belong in this PR, I've removed it.

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Mar 7, 2019

Thanks everyone for your comments, I'm glad it's getting thorough review because I am hesitant to merge something like this on my own.

Before we proceed, we should be clearer on the issue James McCartney has brought up. I found his argument pretty convincing (I hadn't thought of it that way before).

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 7, 2019

@telephon What issue exactly? I believe everything he brought up has already been discussed at length.

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Mar 7, 2019

It was only one issue, namely that the history is lost when you do this. I still don't understand how one could ever brush over such a warning.

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 7, 2019

I think there is a misunderstanding. History is not lost by doing this, it just becomes slightly more difficult to navigate it with git blame. After this change, if you want to find the last commit where a line was changed or introduced, if it lies behind this change, you'll need to checkout the parent of the formatting commit, find the corresponding line, and continue reblaming. As time goes on and there are more fresh commits on develop, the need for that will be less and less (although never quite zero).

However, history is lost and rewritten when you use git filter-branch, which is one approach we've also discussed. All commit hashes on the main branches would change, which means all commit hashes linked to in comments, mailing list, forum posts, etc. would be invalidated, and everyone who has a clone of this repo would have to wipe it away and reclone, or do some careful branch management to maintain their feature branches. That is not what we are doing here.

Hope that clears things up.

@telephon

This comment has been minimized.

Copy link
Member

telephon commented Mar 7, 2019

What I don't understand is why the disagreement on sc-dev couldn't be resolved this way. I suppose that there is something in the process of checking out the parent of the formatting commit, finding the corresponding line, and continue reblaming which is not as straightforward as it seems to be?

There must have been a good reason why James warned.

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Mar 7, 2019

@joshpar

This comment has been minimized.

Copy link
Member

joshpar commented Mar 7, 2019

Right now, we have a lot of commits that DO reformat some existing code already to support a changes that goes in now. In my mind, having one commit that resolves all of this now means less of a problem later. Right now, blame may point to an actual change or possibly some bit of formatting that was done to satisfy a problem with new code. After this goes in, we’ll have one commit that most of would recognize as being ’the great code reformat of 2019’ and that should point us towards looking further back in history if a blame is needed.
I was certainly bitten by this in the past - I’m submitting a PR, but formatting is wrong - so I change it, then need to change code around it, etc. to get the PR in. I may have only changed one or two lines of code for the PR, but now the formatting is forcing me to fix up more. At this point, I would vote to get this done in one pass, and remove that hurdle for future contribution. And I think if anything needs a blame, we’ll know to watch out for this commit as a possible issue.
However - do we use blame that much as a team?

@muellmusik

This comment has been minimized.

Copy link
Contributor

muellmusik commented Mar 7, 2019

Because of human nature? I don't think we should be continuing this discussion any further.

@patrickdupuis Because of human nature, I don't think it's a good idea to try to curtail discussion in this way, however well-intentioned the suggestion. I think it would be good if we take a little care now to avoid the likelihood of any further misunderstandings or misinterpretations.

@telephon The issue with blame is it will be more of a pain. That is the tradeoff. FWIW it's worth, it will be similar to when code has been moved. I find that a little annoying when I'm in a hurry, but there we are. This will not be more annoying I think.

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Mar 7, 2019

@muellmusik Agreed, sorry for the snarky comment. We should be discussing things that were said on the ML over on the ML. The discussion here should be about the PR.

@joshpar

This comment has been minimized.

Copy link
Member

joshpar commented Mar 7, 2019

@shimpe

This comment has been minimized.

Copy link
Contributor

shimpe commented Mar 8, 2019

As long as the reformatting commit consist of nothing but reformatting there's no problem.
Problems interpreting history only occur if you start to mix bug fixes or new features with code reformatting but that is not the intention here.

Using "git gui blame somefile.cxx", makes that blaming the parent commit is literally just right-clicking on a line of code and then clicking "blame parent commit". There's nothing difficult or tricky about the process. Been there, done that a gazillion times.

I've been through similar cleanup efforts at work in much larger codebases, and they only brought happiness really. We now also use a git pre-commit hook to ensure that all code that arrives in the repo is always perfectly formatted. There's absolutely no losing of history or whatever doomsday scenarios have been brought up. Such fears are based on a poor understanding of what is actually going to happen.

@snappizz
Copy link
Member

snappizz left a comment

thanks!

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 8, 2019

Just want to reiterate that even though this is approved, WAIT TO MERGE.

@muellmusik

This comment has been minimized.

Copy link
Contributor

muellmusik commented Mar 8, 2019

There's absolutely no losing of history or whatever doomsday scenarios have been brought up. Such fears are based on a poor understanding of what is actually going to happen.

I’ll repeat my request for a little care in how we word things. This is a misrepresentation of concerns raised by people who do understand the issue. James for example has many, many years of experience programming professionally and has also experienced such changes. The fact that someone prioritises something other than you do does not make their views a ‘doomsday scenario’. You will only inflame the feelings who those who feel differently by using such words. You will not convince them.

@timblechmann

This comment has been minimized.

Copy link
Contributor

timblechmann commented Mar 8, 2019

clang-format seems to become rather hip these days, but i'm not a huge fan of it

  • i've experimented with clang-format a lot when writing supernova. it's a programmer's wet dream, but typically there are edge cases, where the result of the formatting is more than questionable. in my professional life i've seen it in action on a larger codebase. the results are, well, not totally convincing.
  • for a refactoring like this, loosing commit annotations (git blame) is a blocker imho. i've seen code where some people thought it would be great to move code around and change style for no reason. loosing the annotation made it a pain to work with it later on. one can try to rewrite the history via git-filter-branch, with all advantages and disadvantages (invalidating commit references).
  • existing patches won't apply anymore. some people may have their private forks of the codebase (i still have a fork with a few dozen of patches, WFSCollider used to be based on a fork that allowed sample-accurate synchronisation between multiple servers, maybe there are others). porting this to newer versions will be quite a bit of effort, as hardly any patch will apply anymore
@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 8, 2019

@timblechmann This was announced on the ML weeks ago and I got buy-in at that point from most major contributors. Why didn't you voice your concerns then?

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 8, 2019

Re your points:

  • bad edge cases:
    • still worth not spending time in review arguing over formatting
    • still worth less experienced contributors being able to format their code without thinking too much about it
    • clang-format pragmas if needed
    • llvm is under active development, so edge cases are likely to improve and will be fixed incrementally in new patches
  • git filter-branch: this has already been addressed above
  • invalidating patches: would happen with any approach, but not too difficult to get your work rebased deterministically. and, only happens roughly once per C++-changes branch rather than for every branch of every repo with git-filter-branch
@scztt

This comment has been minimized.

Copy link
Contributor

scztt commented Mar 8, 2019

I think @joshpar's point is quite good: unless we discourage any reformatting (which I don't think ANYONE wants), then formatting clean-up will happen eventually. This will always be a problem for blame - given that, it feels much better to deal with once rather than spread the change over many commits + months of time.

One more thing, because it keeps coming up: this work was discussed and planned extensively, with lots of opportunity for commentary and response on the mailing list, github, the scsynth forum, and the regular open development meetings. I think can be very discouraging to those who HAVE been weighing in on this to have a chorus of voices join in only at the 11th hour, at the (quite literal....) last possible minute.

This thread should stick to technical discussions regarding the change at hand - but I think it would be good to have a separate conversation about how we can make large-scale decisions like this one, and make sure all stakeholders feel like they have a chance to weigh in. In particular, I'd like to hear from people who felt like they had something to contribute to this effort or should have been involved more in conversations, but e.g. didn't know what was going on, or were excluded in some way that others might not be sensitive to. I'll post something on sc-dev about this topic - let's discuss there (and again, let's also keep this thread for discussing details of the reformatting work rather than rehash of decision-making leading up to it).

@muellmusik

This comment has been minimized.

Copy link
Contributor

muellmusik commented Mar 8, 2019

I think can be very discouraging to those who HAVE been weighing in on this to have a chorus of voices join in only at the 11th hour, at the (quite literal....) last possible minute.

I know, and I have felt such frustration myself. But people are busy, and may not have noticed or have been able to respond at the time. That does not make their opinions invalid, or deprive them of the right to contribute to the discussion. Nobody should be made to feel like they can't have a view just because they weren't able to do it at a convenient moment.

A little respect all around can help with that in both directions, but let's face it: This was always going to be a contentious change, and anyone pushing for it needs to accept that. It will continue to be for a long time if it goes through. That's not going to change.

Honestly when I think of the amount of bad feeling generated by issues of formatting and whitespace I want to despair for humanity... ;-)

@brianlheim

This comment has been minimized.

Copy link
Member Author

brianlheim commented Mar 8, 2019

@scztt and @muellmusik both make good points. And @timblechmann, despite my initial gruffness, I am grateful that you shared your concerns. I hope everyone's interests here can be resolved peacefully and amicably.

@timblechmann

This comment has been minimized.

Copy link
Contributor

timblechmann commented Mar 9, 2019

@timblechmann This was announced on the ML weeks ago and I got buy-in at that point from most major contributors. Why didn't you voice your concerns then?

i'm not following the ML and don't consider myself as major contributor.

  • clang-format pragmas if needed

not a big fan of cluttering the codebase (been there, done that)

  • llvm is under active development, so edge cases are likely to improve and will be fixed incrementally in new patches

i was hoping for the same ... like 6-7 years ago. but it's not necessarily a problem of the tool, more that the c++ grammar is rather complex and tools have less understanding of the semantics. formatting lisp (even sclang) is much easier, as the grammar is simpler.

  • invalidating patches: would happen with any approach, but not too difficult to get your work rebased deterministically. and, only happens roughly once per C++-changes branch rather than for every branch of every repo with git-filter-branch

if you have N patches, you need to touch N patches ...

--

fwiw, i'd still advocate the old quote:

“Programs must be written for people to read, and only incidentally for machines to execute.”

by abelson/susmann. in my experience clang-formatted code tends to reverse this.

@scztt

This comment has been minimized.

Copy link
Contributor

scztt commented Mar 9, 2019

@timblechmann Very valid points. I think we need to consider the use of clang-format as a preliminary step forward to solve some very real problems around making it easier to accept code contributions and keep quality, coherence, readability. Every code change that goes into SC these days is reviewed, so there will be plenty of opportunity to discuss formatting issues in context and revisit both clang-format and code style issues in general. I think this will receive very close attention in the next weeks/months, and there's a lot of room to course correct as things arise.

I would encourage anyone who feels invested in formatting / code hygiene issues to spend some time following pull requests in the coming months - both to help out shepherding changes / triaging issues, and to get a better sense of how all this effects contributors in real-world, non-hypothetical settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.