Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Nov 27, 2014

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.

@sipa
Copy link
Member Author

sipa commented Nov 27, 2014

@Diapolo early christmas present.

@TheBlueMatt
Copy link
Contributor

Another way to title this would have been "BREAK ALL THE MERGES!".
Still, concept ACK, I'll check this next week...

@jonasschnelli
Copy link
Contributor

Nice work.
Idea for improvements:

  1. Could the script be parameterized to just clang-format the files which are in the current tracked by git?
  2. Could the script be parameterized to just clang-format the files of a specific commit?

ACK on the script.
Did not review the files.

@sipa
Copy link
Member Author

sipa commented Nov 27, 2014

  1. Could the script be parameterized to just clang-format the files which are in the current tracked by git?

Sounds like a good idea, but not a blocker imho.

  1. Could the script be parameterized to just clang-format the files of a specific commit?

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.

@jonasschnelli
Copy link
Contributor

Could the script be parameterized to just clang-format the files which are in the current tracked by git?

Sounds like a good idea, but not a blocker imho.

Yes. Totally non-blocking-this-pull idea.

@laanwj laanwj added this to the 0.10.0 milestone Nov 28, 2014
@sipa
Copy link
Member Author

sipa commented Nov 28, 2014

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):

deb http://llvm.org/apt/trusty/ llvm-toolchain-trusty-3.5 main

@laanwj
Copy link
Member

laanwj commented Nov 28, 2014

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.

@dexX7
Copy link
Contributor

dexX7 commented Nov 30, 2014

I got my clang-format-3.5 from (apt sources line)

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.

@sipa
Copy link
Member Author

sipa commented Nov 30, 2014

@dexX7 Was the result identical?

@sipa
Copy link
Member Author

sipa commented Nov 30, 2014

@dexX7 Also, no need to move it. You can run it from the root. (i.e., ./contrib/devtools/auto-format.sh).

@dexX7
Copy link
Contributor

dexX7 commented Nov 30, 2014

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?

@sipa
Copy link
Member Author

sipa commented Nov 30, 2014

They're not?

EDIT: Ugh, they seem to be.

@laanwj laanwj modified the milestones: 0.11.0, 0.10.0 Dec 11, 2014
@laanwj
Copy link
Member

laanwj commented Dec 11, 2014

Bumped to 0.11.

@Diapolo
Copy link

Diapolo commented Dec 12, 2014

Why?

@laanwj
Copy link
Member

laanwj commented Dec 12, 2014

@Diapolo Please read the above discussion. Clang-format needs more time to stabilize.

@sipa
Copy link
Member Author

sipa commented Apr 8, 2015

Closing as highly outdated.

@Diapolo
Copy link

Diapolo commented Oct 8, 2015

Should be done finally...

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants