Formatting recently added files likely does not create many conflicts.
Reviewers may use
contrib/devtools/clang-format.py ~/clang-format-3.6.2 src/bench src/http* src/qt/bantablemodel.* src/test/addrman_tests.cpp src/test/dbwrapper_tests.cpp src/test/streams_tests.cpp src/zmq/zmq*This will clean up the new files from #6733, #6103, #5677, #6650 and #6315.
UPDATE:
I have appended a commit to set AlignAfterOpenBracket to false.
Set AlignAfterOpenBracket: false
@dcousens I did only clang-format recently added files to prevent creating merge conflicts and see if any updates are necessary with regard to the clang-format workflow.
You can find the set of files in the OP (hidden in the command line)
Please discuss and schedule this at the next IRC meeting.
Similar to the libconsensus reformats, there is no point creating a pull request for cosmetic, mechanical changes unless they are ready to be merged soon after creation.
If cosmetic changes will not be merged immediately, this creates a constant-rebase hell for the pull request in question.
Given that it takes mere minutes to re-create this from scratch, it is better to
@MarcoFalke is there perhaps a way we can put this under test? Making maintainers conform to a style that would otherwise be a breaking test is a great way to avoid discussion/bike shedding over this.
Perhaps something to bring up at the IRC meeting.
next IRC meeting
In case this means http://www.wolframalpha.com/input/?i=15+utc+thursday #bitcoin-core-dev , I am already scheduled.
127@@ -127,7 +128,7 @@ static bool HTTPReq_JSONRPC(HTTPRequest* req, const std::string &)
128 // Send reply
129 strReply = JSONRPCReply(result, NullUniValue, jreq.id);
130
131- // array of requests
132+ // array of requests
@laanwj I think the sooner we can just put this under test, the bike-shedding and nitting will just disappear because the tests will spell it out, and it just becomes a non-issue. Yes, it will make a few commits that may appear trivial [initially], but, in the interest of the long term readability of the code base, I think it would be worth it. YMMV.
If there is any discussion of this at the IRC meeting, it should probably be directly in response to #6839 (comment), not necessarily the PR itself.
To be clear, my position is
A couple thoughts:
When changing existing code we must not just trust clang-format repeatability to verify that no bugs were introduced, we should test via checks for identical object files. This will take a little effort because some macros insert line numbers, and these will need to be bypassed.
To use clang format normativeily we must end up specifying a particular version that all contributors must use, as clang format changes its behavior. This is irritating, and disruptive to contributing. It could be made much better with good automation like cfield’s depbuilder work, to help people get a particular version of clang-format; though due to the flowing API breakage in C++ code, I don’t know how long an older version will be maintainable.
Another concern is which style to specify. The most common behavior in Bitcoin Core today has stylistic elements which have been objectively shown to increase software defect rate and confuse review– e.g. unbraced multi-line IFs. We should not make any disruptive reformating changes unless we are quite confident that the style is what we want to live with.
We could avoid disrupting the workflow of existing patches by only reformatting (and then requiring it) on new files and substantial rewrites. I think that would be a better path.
To the extent that there is hope to address bikeshedding via this, I beg anyone thinking that to give up that thought. Bikeshedding is not a result of any particular coding style or consistency, bikeshedding arises from a lack of perspective which can only be addressed by experience, community norms, and having more important things to worry about.
Stylistic consistency has actual benefits; it aids newcomers in their contributions because it is easier for them to make sure their work is okay on styleistic grounds; though this may be offset by having to install some particular version of clang-format before they can get started. It eases review because the uniformity creates better expectations; but reformating makes looking at the history harder, which harms review. Good style choices (as opposed to merely consistent) have, at times, been shown to lower defect rates in software– but there is not a universal opinion on what choices are good. So there are benefits to be had, but they must be carefully managed with other risks and costs. And the magnitude of the potential benefits, I think, are enough to say that this subject should have a very low priority in general.
@gmaxwell To use clang format normativeily we must end up specifying a particular version that all contributors must use
https://github.com/bitcoin/bitcoin/blob/master/contrib/devtools/clang-format.py#L15
@gmaxwell We could avoid disrupting the workflow of existing patches by only reformatting (and then requiring it) on new files and substantial rewrites. I think that would be a better path.
I agree, but I am not sure about the requirement. Imagine changes to critical code, refactor-only or just a new feature whose PR aims to produce a simple diff that is easy to read and to verify. Often clean diffs and proper formatting are orthogonal.
Entirely new files are the exception but they should ideally be formatted before submitting the PR (at least prior to merging).
I think, are enough to say that this subject should have a very low priority in general.
Yes, and with that, I’m going to close this.