-
Notifications
You must be signed in to change notification settings - Fork 38.7k
clang-format all the things! #5387
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
Conversation
|
@Diapolo early christmas present. |
|
Another way to title this would have been "BREAK ALL THE MERGES!". |
|
Nice work.
ACK on the script. |
Sounds like a good idea, but not a blocker imho.
Yes, that would be interesting as an option. However, I think we want to get into a state where most of the code, most of the time, is mostly according to clang-format standards already, and re-applying the script (it takes 1.2s here) to the whole tree should be mostly a no-op anyway. On the other hand, if the resulting state is that there are always a few commits merged in that broke coding style, you don't want to independent pull requests to go clean those up automatically. Your suggestion is one way to avoid that. Another is to just periodically (and perhaps automatically) have a pull request created that applies coding style fixes. |
Yes. Totally non-blocking-this-pull idea. |
|
So I think a requirement here is that the formatting can be reproduced by others. I got my clang-format-3.5 from (apt sources line): |
|
Concluding from the discussion on IRC I think it would be better to postpone this until clang-format is more stable. Even different versions within 3.5 give different results - or at least: the clang-format-3.5 in Ubuntu 14.04 refuses to accept the input file completely, and in 3.6 some more things change (see the diff in my above post). To make this repeatable we would have to lock in one specific commit tag of clang/LLVM. That kind of defeats the advantages of being able to do this (and check this) automatcially. |
It can also be extracted from the binary packages from http://llvm.org/releases/download.html and it is available as /bin/clang-format (e.g. clang+llvm-3.5.0-x86_64-linux-gnu.tar.xz -> ./clang+llvm-3.5.0-x86_64-linux-gnu/bin/clang-format). It worked for me on Ubuntu 14.04 after moving auto-format.sh into the root dir and replacing "clang-format-3.5" by the actual formatter file I have somewhere else. |
|
@dexX7 Was the result identical? |
|
@dexX7 Also, no need to move it. You can run it from the root. (i.e., ./contrib/devtools/auto-format.sh). |
|
Yes, that seems to be the case. I reverted your last commit and run it here: dexX7@d328f6e May I ask why headers are excluded? |
|
They're not? EDIT: Ugh, they seem to be. |
|
Bumped to 0.11. |
|
Why? |
|
@Diapolo Please read the above discussion. Clang-format needs more time to stabilize. |
|
Closing as highly outdated. |
|
Should be done finally... |
Included is a script that will automatically format every file. Several files and directories are excluded for now, either because they come from a different project, because their coding style is significantly different already, or the result is just too terrible to see.
Also note the trick used in bloom.cpp to avoid the ugly all-constructor-arguments-on-one-line that sometimes happens.