[WIP] tools: use clang-format to format C++ code

[joyeecheung]

I think we have discussed briefly about this during the collaboration summit @BridgeAR @Fishrock123 This is just an attempt to see how everybody feels about using an automatic formatting tool to reduce style inconsistencies and nits in PRs. We use clang-format in llnode and v8 uses it as well so this sounds like a good candidate.

• The first commit just initializes the.clang-format file based on Google styles (since previously we use cpplint.py that lints C++ code using the Google style guide). We have our own "extensions" to the style guide (e.g. the ones being added in doc: add basic C++ style guide #16090) so we need to work out more rules in the .clang-format based on our needs.
  • If we want more granular control over the rules we can do a clang-format -style=google -dump-config > .clang-format and start from that as well.
  • This PR is first about seeing if there are any concerns on the idea of using a auto-formatting tool at all. And the rules will kinda depend on what doc: add basic C++ style guide #16090 comes down to.
• The second commit formats every .cc under src with the initalized .clang-format (clang-format -style=file -i src/**/*.cc)
• I am using the clang-format npm module with pre-built binaries maintained by the Angular team, though clang-format can be obtained by compiling from the clang source code as well. Also you can get a python wrapper if you happen to have depot_tools and a V8/Chromium checkout on your machine. I am not sure whether we should bundle it in the codebase like what we do with eslint, but definitely doesn't hurt to add some commands to the Makefile (maybe along with eslint --fix).

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

src, tools

[joyeecheung]

cc @addaleax @TimothyGu @bnoordhuis

[danbev]

Should this be applied to test/cctest as well perhaps?

[bnoordhuis]

Yes, and test/addons and test/addons-napi as well.

The problem of course is that it results in humongous diffs. It:

1. creates merge conflicts with just about every open pull request that touches C++ code, and
2. makes back-porting hard unless it's merged simultaneously in all branches;

I am using the clang-format npm module with pre-built binaries maintained by the Angular team

That might be a good solution, because

3. the problem with just any clang-format is that different versions behave differently.

[joyeecheung]

@danbev @bnoordhuis Just noticed I have missed src/**/*.h as well. Added to the commits.

1. creates merge conflicts with just about every open pull request that touches C++ code, and
2. makes back-porting hard unless it's merged simultaneously in all branches;

I think we can try to relax the rules a bit and make them more about capturing what we have right now, then tighten the rules one by one later, so there will be less conflicts to start with. Basically by doing

If we want more granular control over the rules we can do a clang-format -style=google -dump-config > .clang-format and start from that as well.

That I've described in the OP. I will try to tweak with it a bit, maybe after #16090 lands.

3. the problem with just any clang-format is that different versions behave differently.

Yeah the npm module does keep track of the revision of its binaries. Also we can do something like what make coverage does, use the bundled npm and install it in the project directory.

[joyeecheung]

Did a bit of archaeology and found #1539 , let's see if clang-format is flexible enough to do this kind of stuff again

[joyeecheung]

#16090 has already landed. Will try to tweak the .clang-format over the weekend.

[gibfahn]

The problem of course is that it results in humongous diffs. It:

• creates merge conflicts with just about every open pull request that touches C++ code, and
• makes back-porting hard unless it's merged simultaneously in all branches;

I think we can try to relax the rules a bit and make them more about capturing what we have right now, then tighten the rules one by one later, so there will be less conflicts to start with. Basically by doing

From an LTS/backporting perspective if we're going to do this I'd rather do it all at once, and backport it ASAP, rather than the "death by a thousand cuts" style of making changes. I assume that's better for open Pull Requests too.

[joyeecheung]

@gibfahn hmm now come to think of it, I think this one can be backported by running the final clang-format on all branches instead of manually backporting them? Also for conflicts in other PRs we can do that as well, so it might not be that hard... I will give this another try over the weekend

[joyeecheung]

BTW: I am still pursuing this. One outstanding issue is when is appropriate for this to land, if it ever gets landed. Any patches to C++ code landed prior to this patch would not land cleanly anymore, even when this patch is backported to different release lines asap, and the conflict would look pretty scary. To land those C++ changes one has to manually recreate the commits by copying files via git checkout, do a make format, and commit again, instead of cherry-picking the commits. So it's best to land this when a minor release on all release lines is out and the number of C++ commits on master that have not been backported is minimum.

[bnoordhuis]

@joyeecheung http://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting - can be used to reformat commits when applying them. The CI can probably be made to do it automatically when rebasing onto the target branch.

[joyeecheung]

@bnoordhuis Ooh, that looks pretty nice! Thanks for discovering that. I'll see if I can integrate it somehow.

[BridgeAR]

@joyeecheung is this something you still work on? I believe it would still be great to have something like this.

[bnoordhuis]

FWIW, I still think it's a good idea and the practical issues are tractable.

[joyeecheung]

@BridgeAR I have not forgotten about this, just need to find some time to tweak this again...

[mmarchini]

If I'm not mistaken, git cl format on v8 will only format modified files (not sure how they determine "modified" though). Maybe we could use a similar approach to minimize breakage among PRs and backports?

[joyeecheung]

Closed in favor of #21997