Skip to content

tools: add make format-cpp to run clang-format on C++ diffs#21997

Closed
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:clang-format-diff
Closed

tools: add make format-cpp to run clang-format on C++ diffs#21997
joyeecheung wants to merge 1 commit intonodejs:masterfrom
joyeecheung:clang-format-diff

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jul 27, 2018

This is a rework of #16122. I took the advice from @codebytere and make clang-format run on C++ diffs instead of the whole code base (this is also what chromium's git-cl does). This allows us to gradually format the code base while we update the C++ files in normal PRs (although it may require support from external tooling and build to make sure people run it before landing PRs).

There is currently only a shortcut in Makefile. To run it on Windows one will either need to start the command manually or refactor vcbuild.bat a bit to include/exclude the formatable C++ files in the command.

One thing to note: the clang-format npm package is maintained by the Angular team. It directly downloads executables hosted in their repository: https://github.com/angular/clang-format It seems to be reasonably safe to use. Otherwise we will have to build and host a bunch of executables for different platforms ourselves because different versions of clang-format may format the files differently so we would want to keep the version fixed.

------ patch description ----

This patch adds a make format-cpp shortcut to the Makefile
that runs clang-format on the C++ diffs, and a
make format-cpp-build to install clang-format from
npm.

To format staged changes:

$ make format-cpp

To format HEAD~1...HEAD (latest commit):

$ CLANG_FORMAT_START=`git rev-parse HEAD~1` make format-cpp

To format diff between master and current branch head (master...HEAD):

$ CLANG_FORMAT_START=master make format-cpp

Most of the .clang-format file comes from running

$ clang-format --dump-config --style=Google

with clang-format built with llvm/trunk 328768 (npm version 1.2.3)

The clang-format version is fixed because different version of
clang-format may format the files differently.

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

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants