Format first-party source according to .clang-format #3178
Conversation
thejohnfreeman
force-pushed
the
clang-format
branch
from
d7bac21 to
00f5750
Compare
|
The first thing I notice is that this breaks the build. Fortunately, it was only a couple of quick changes to fix it: ximinez@466dfb4 |
|
I merged @ximinez's fixes. This PR is only 3 commits ahead of ripple:develop (not 19), which you can see at develop...thejohnfreeman:clang-format. Fingers crossed that GitHub will recalculate that difference soon. |
Codecov Report
@@ Coverage Diff @@
## develop #3178 +/- ##
===========================================
- Coverage 70.41% 70.21% -0.20%
===========================================
Files 683 683
Lines 54775 54621 -154
===========================================
- Hits 38570 38354 -216
- Misses 16205 16267 +62
Continue to review full report at Codecov.
|
|
@nbougalis raised the issue of multi-line if-conditions branching to non-compound bodies. That is, an if-statement with a multi-line condition and a "single statement" body with no curlies. Here's an example, as our if (loadFactorServer != b.loadFactorServer ||
loadBaseServer != b.loadBaseServer || baseFee != b.baseFee ||
em.is_initialized() != b.em.is_initialized())
return true;Nik and I both find it undesirable that the first column of the body statement is aligned with the first columns of the continued lines of the condition expression. Let's call this the Binary Operator Scenario. Note that there are two scenarios in which these columns do not perfectly align:
if (!otherMap.walkBranch(
other, ours->peekItem(), false, differences, maxCount))
return false;
if (app_.timeKeeper().now() < (current->info().parentCloseTime +
2 * current->info().closeTimeResolution))
{The only clang-format option that has any effect on the Binary Operator Scenario is My personal preference is to see every if-statement with a multi-line condition use a compound statement for its body. The opening curly does a good job of marking the end of the condition and the start of the body. However, clang-format never adds any non-whitespace tokens; it will never change a non-compound statement to a compound statement. To make this kind of change automatically, we would have to use clang-tidy. Lastly, for a particularly egregious example that would look much better with a compound statement, take a look at lines 314-323 in src/ripple/protocol/impl/STParsedJSON.cpp: if (!(0u <= type &&
type <=
std::min<unsigned>(
std::numeric_limits<
std::uint16_t>::max(),
std::numeric_limits<
std::underlying_type_t<
LedgerEntryType>>::max())))
Throw<std::runtime_error>(
"Invalid ledger entry type: out of range"); |
thejohnfreeman
force-pushed
the
clang-format
branch
from
4971785 to
fef38d1
Compare
I would take this a step further even. My preference is that every if statement, whether the condition is multi-line or not, use a compound body, even when it's just a one line body. I know it's not necessary, but just a personal preference. |
|
@thejohnfreeman can we update this and actually merge it? My past two PR's have extraneous reformatting that is a pain to avoid (and a pain for reviewers). Would like to merge this if possible. If we decide we want to change the .clang-format file in the future, we can just reformat again and add the commit to .git-blame-ignore-revs. |
|
I'm in favor of merging. It might be worth doing a beta with this all by itself so we can get it over with quickly. |
|
I'll prepare a new commit based on 1.5.0-b5 and update this PR. If I can get it passing tests quickly enough, maybe we can talk @manojsdoshi into creating a b6 beta soon. |
thejohnfreeman
force-pushed
the
clang-format
branch
2 times, most recently
from
fe9c825 to
1ac91cc
Compare
|
Not all sources seem to be formatted. For example, |
|
It would be good to add a CI flags any mis-formatted sources. We should probably do that in this PR. |
|
Since we're reformatting everything, we should consider:
|
|
The reformatting changes are still showing up in my git blame. Edit:
|
|
I'd prefer to make this the first (and only) commit of a 1.6.0-b1 rather than put this into 1.5. |
|
Not sure how so many source files slipped past without getting formatted. Those changes are incoming. I put the Git version requirement in the log message, but I'll add it and some instructions to the ignore file. |
thejohnfreeman
force-pushed
the
clang-format
branch
from
1ac91cc to
e9016f1
Compare
|
Most coding style guides I have seen in the past - including Google's are way too big for anyone to fully absorb. I think this is great that we are converging to a style via .clang-format. My comments are as follows, note they do not necessitate any action items for this PR, perhaps just some thoughts,
|
|
I had put this branch up two months ago hoping it would give people a chance to easily check it out and see if they dislike the style or if it screws up their editor, but I'm not sure anyone took the opportunity. It might be better to ask forgiveness than permission in a sense; it might be the only way to get folks to check it out. We can always make fixes after, or even roll back the change entirely, as part of the beta refinement process. I would like to see a CI job as well, but it can be developed separately. I'll open an issue and link it to this review. Hooks are local to a repository, so I'm not sure how we can deploy a clang-format hook for everyone to use. |
|
You can use the https://pre-commit.com/ tool to make sure everyone uses the same version of clang-format (you can also run it in CI) and then it is up to everyone individually to install a pre-commit hook already locally or have faster commits and have the CI shout at you in case you didn't do it right in the first place. People tend to prefer different work styles and this enables both. |
|
I dont know if we need to really do anything in terms of deploying a clang-format hook. There are many different ways to use clang format correctly. Most editors provide clang-format integration. You can run git-clang-format from the command line as well, which reformats any staged changes and adds them to the working tree. We could provide an example pre-commit hook in the repo if you want, that people can use if they wish (by copying to local .git/hooks). As @MarkusTeufelberger said, people tend to prefer different work styles, and I am not sure we really need to provide a way to use it. There is extensive documentation online for clang-format as well. I found a way to add this to CI by the way, via github actions: https://github.com/cjcobb23/rippled/blob/develop/.github/workflows/ccpp.yml |
|
I added the GitHub action workflow. It ran as expected on my fork: https://github.com/thejohnfreeman/rippled/runs/465667385 |
thejohnfreeman
force-pushed
the
clang-format
branch
from
870a47f to
d4cf33b
Compare
|
This just needs some approvals before @manojsdoshi can cut a beta with it. What do we think? @nbougalis @seelabs |
|
@thejohnfreeman @manojsdoshi There is a proposed b6: #3270 . This PR looks great to me, but if that b6 goes in, we have to rebase and reformat. I actually vote for tacking this on as the last commit in the proposed b6. If we push the proposed b6 to b7, and cut this as a b6, all of the PR's in the proposed b6, which are all approved, are going to have to rebase and reformat. I'd also rather not wait to merge this until a b7, or 1.6, because this PR has been open for quite some time now, yet merging it keeps getting pushed back. @seelabs Since this doesn't actually introduce any functionality changes, I don't think we need to have this as the sole commit for a beta; I think it's fine to include this in a beta with other commits. What do we think? |
|
Fair enough, though that means b1 will not be an ancestor of b2. Not sure if that bothers anyone, but I do think it would be new. |
|
Feel free to ignore me since I won't be involved with any of this. But, I would be wary of rebasing on top of the formatting PR. I could be wrong, but git doesn't always handle this in the best way and it could end up being a little more difficult than anticipated. There's also a risk of things going awry when rebasing. John's plan seems, at least intuitively, a bit safer: include Tickets in 1.6.0-b1, and then have b2 include this PR exclusively. Formatting is just running an automated tool; rebasing is manual and at risk of human error. |
|
Re-reading my last comment, I see now that it probably made no sense to anyone else because I misunderstood @cjcobb23's last comment. Here's what was going through my mind, which may be an option for (a) merging this format PR ASAP and (b) making it easy to merge the Tickets PR:
|
thejohnfreeman
force-pushed
the
clang-format
branch
4 times, most recently
from
18d83ef to
85e8e2f
Compare
|
@thejohnfreeman I just checked the PR out and ran clang format against src/ripple and looked for any files that changed. It looks like we missed some of the ipp files. So we need to format these and change the github action to check for these files as well. Here's a patch that should work: seelabs@3173fe0 Once we take care of these ipp files I think this is ready to go and we'll do a beta. |
thejohnfreeman
force-pushed
the
clang-format
branch
2 times, most recently
from
76236aa to
fcd322f
Compare
.git-blame-ignore-revs
Outdated
| # This feature requires Git >= 2.24 | ||
| # To use it by default in git blame: | ||
| # git config blame.ignoreRevsFile .git-blame-ignore-revs | ||
| 09a4679b9eaafb430d1c19a83e5c3143ea34b745 |
There was a problem hiding this comment.
Sorry I missed this, but I think this needs to be updated now too.
There was a problem hiding this comment.
Good catch. I pushed the fix. Can't wait for this to go in so I can stop recreating this changeset...
thejohnfreeman
force-pushed
the
clang-format
branch
2 times, most recently
from
199bda6 to
bc01a9d
Compare
- Add missing `#include` in `ripple/core/JobTypeInfo.h` - Protect version string from clang-format in `ripple/protocol/impl/BuildInfo.cpp`. `Builds/CMake/RippledVersion.cmake` searches for this line by pattern.
thejohnfreeman
force-pushed
the
clang-format
branch
2 times, most recently
from
bc01a9d to
331472d
Compare
This is not a PR I expect to be merged. I'm just creating it to give us all a chance to look at what clang-format would do to our code based on the
.clang-formatoff thebranch developPR for 1.5.0-b1 (#3167). The diff is huge. I suggest you pick out your favorite files and see if you like the outcome.I ran clang-format on all the source files ending in
.hor.cppundersrc/rippleandsrc/test. I left the rest alone. There's plenty to see here without getting into special case files, and I don't think we want to worry about formatting third-party code.The top ten most-changed files, where a "change" is an added or removed line:
clang-format detected these files as Objective-C and reported them with the warning "Configuration file(s) do(es) not support Objective-C: rippled/.clang-format". It left them unchanged. I could not find an option to clang-format to force the language, and the documentation suggests it is based off the filename. These are all headers, so it probably digs into the code to try to disambiguate.