Open scravy opened 6 years ago
This issue is a bit old so we should get to a resolution here (implement soon / schedule to a future milestone / close). I like the general idea of having a consistent style cross the entire project but I'm not a big fan of making such a wide codebase change before getting some more mileage with merging Bitcoin Core future versions and seeing how is it to work on the merged codebase. One use case I can think about, that enjoys an easy approach without any "code transformations" is cherry-picking commits from upstream as was done here and here
I would very much appreciate a consistent code base and I do not mind a huge code change, as it is purely syntactical (if clang-format
is used it literally only moves white space, by design). With the changes we are doing by virtue of the clone machine we are already doing pretty extensive changes.
There's some pros and cons though:
pro: We can activate the stylistic checks on pull requests for the whole repository and hopefully do not have to argue/review anything in pull requests relating to "does the curly brace go on this line or the next line"
pro: I am a firm believer that this actually makes some merges easier (I am talking about the upstream syncs with bitcoin releases). For example:
We (or bitcoin) change something from if(x){...}
to if (x) {...}
(inserted spaces, formatting). This would not produce a merge conflict in any way as the clonemachine would always transform bitcoin into a canonical version before merging (and our code should always be in that shape).
cons: We lose some history. That is actually sounds too big: we do not really lose the history, but you can't just look at a line and always be sure to understand who touched it most recently (it might say "clonemachine" instead of "J. Random Hacker").
cons: It's harder to cherry-pick commits from bitcoin. Quite frankly I do not mind that too hard as this should not happen too often. Also when cherry-picking commits we frequently need to adjust something later on anyway to integrate it properly, just in our case there would be an additional step of applying formatting, which I think is fine. It's actually the same step as we already have with the clonemachine.
The biggest cons argument in my opinion is the one about history, but then again we're not really losing history, it just requires an extra step and you can't just git blame
something as easily as before.
--
Having said that, there's some things which just apply clang-format, be it as part of the clone machine or not, does not take care of:
clang-format
literally only touches white space. It does not insert curly braces around single-statement blocks for example.clang-format
(since it only touches white sapce) can not be adjusted for for example make comments homogeneous (right now there's a mixture of /**
and //!
and I would love to see the process we're talking about to also turn all doxygen comment into the canonical version of //!
only).I tend to agree with @scravy that "normalizing" the code regarding formatting for the merge will make merging easier. I also see it as pretty much the only way to get to cleaner code without detaching from upstream. It is an imperfect solution, though, as it doesn't cover everything we would want to clean up, but a small win still is a win, isn't it?
One thing we probably would need to do when merging branches formatted with clang-format is to use the ignore-all-space
option as in git merge -Xignore-all-space master
to avoid conflicts due to the formatting changes.
In accordance with ADR-6 https://github.com/dtr-org/unit-e-docs/blob/master/adrs/2018-08-27-ADR-6%20Adpot%20Google%20C%2B%2B%20Style%20Guide.md we want to format the codebase using google code style.