tools: add `make format-cpp` to run clang-format on C++ diffs by joyeecheung · Pull Request #21997 · nodejs/node

joyeecheung · 2018-07-27T09:18:36Z

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

nodejs-github-bot · 2018-07-27T09:18:37Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/363/pipeline

targos · 2018-07-27T09:28:01Z

Is it possible to force it to format all files in src (just to see the result)?

joyeecheung · 2018-07-27T09:35:17Z

@targos Sure, I can open another PR to show the diff.

joyeecheung · 2018-07-27T09:44:52Z

@targos See #21998

joyeecheung · 2018-07-27T09:50:16Z

Also: if we managed to implement a Jenkins job that lands a PR for us (squashing all commits just for simplicity) we can run this in the job prior to landing. Another way to use this is to put this in a pre-commit or pre-push hook installed by node-core-utils (see nodejs/node-core-utils#160). CLANG_FORMAT_START is necessary only because the tools don't know where the diff should start from, but ncu should be able to have that knowledge similar to git-cl.

BridgeAR · 2018-07-27T10:00:09Z

This should likely be 80 instead (we use that as the current limit).

Sign in to view

BridgeAR · 2018-07-27T10:03:32Z

Is this necessary? Right below there is the option UseTab: Never.

I guess it's just clang-format's dump option dumping every possible configuration (including the defaults ¯\(ツ)/¯) so yeah why not just keep it to be explicit.

BridgeAR · 2018-07-27T10:09:40Z

It seems like this should likely be set to false. At least in the diff it seems like a lot of arguments are always on a new line (and that is also somewhat nicer to read than having them all on a single line).

There will be a lot of lines changed with this set to either true or false - we have a lot of lines with either styles. I think I measured the lines changed (with either option) at one point when I was working on #16122 and the conclusion was..it doesn't really make a difference

addaleax

Nice :)

addaleax · 2018-07-27T10:09:13Z

I’d prefer for these two to be false … the resulting code is less dense, and I think it aligns better with what we’ve been doing so far

addaleax · 2018-07-27T10:11:06Z

Just curious, is this for the N-API C test files?

It's produced by clang-format's dump option, which basically just lists every configuration including the defaults - the defaults/Google styles change between different version of clang-format so I think it would be better if we list them out instead of depending on those.

Trott · 2018-07-27T19:54:58Z

@nodejs/build-files

TimothyGu

I'm very impressed with how good clang-format is. I echo @addaleax and @BridgeAR on BinPack* but this PR in its current form is already a huge net plus.

What is slightly concerning is that cpplint seems to conflict with clang-format. See https://ci.nodejs.org/job/node-linter/344/console. Could you take a look?

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.

joyeecheung · 2018-08-01T10:13:02Z

@BridgeAR @addaleax @TimothyGu Thanks for the reviews, I've tweaked the .clang-format a bit more according to the feedback:

diff --git a/.clang-format b/.clang-format
index 3461ebaf45..4aad29c328 100644
--- a/.clang-format+++ b/.clang-format@@ -18,8 +18,8 @@ AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: true
-BinPackArguments: true-BinPackParameters: true+BinPackArguments: false+BinPackParameters: false
BraceWrapping:
AfterClass: false
AfterControlStatement: false
@@ -44,14 +44,14 @@ BreakConstructorInitializersBeforeComma: false
BreakConstructorInitializers: BeforeColon
BreakAfterJavaFieldAnnotations: false
BreakStringLiterals: true
-ColumnLimit: 79+ColumnLimit: 80
CommentPragmas: '^ IWYU pragma:'
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: true
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
-DerivePointerAlignment: true+DerivePointerAlignment: false
DisableFormat: false
ExperimentalAutoDetectBinPacking: false
FixNamespaceComments: true

I've fixed a few linter failures in #21998 where it makes sense (see the last commit of that PR). Most of them were caused by NOLINT being misplaced after lines were broken by clang-format. Others were caused by clang-format not being able to format long string literals with macros well within 80 characters given the limitation of its penalty calculation (even with PenaltyExcessCharacter: 1000000). I don't see a good way out for the remaining failures:

src/node.h:73: Weird number of spaces at line-start. Are you using a 2-space indent? [whitespace/indent] [3]
src/node.h:81: Weird number of spaces at line-start. Are you using a 2-space indent? [whitespace/indent] [3]
src/node_version.h:69: Weird number of spaces at line-start. Are you using a 2-space indent? [whitespace/indent] [3]
src/node_version.h:70: Weird number of spaces at line-start. Are you using a 2-space indent? [whitespace/indent] [3]

Which were caused by:

diff --git a/src/node.h b/src/node.h
index a94612eb0b..34946a7cae 100644
--- a/src/node.h+++ b/src/node.h@@ -23,31 +23,31 @@
#ifdef __clang__
-# define NODE_CLANG_AT_LEAST(major, minor, patch) \- (NODE_MAKE_VERSION(major, minor, patch) <= \- NODE_MAKE_VERSION(__clang_major__, __clang_minor__, __clang_patchlevel__))+#define NODE_CLANG_AT_LEAST(major, minor, patch) \+ (NODE_MAKE_VERSION(major, minor, patch) <= \+ NODE_MAKE_VERSION(__clang_major__, __clang_minor__, __clang_patchlevel__))
#else
-# define NODE_CLANG_AT_LEAST(major, minor, patch) (0)+#define NODE_CLANG_AT_LEAST(major, minor, patch) (0)
#endif
#ifdef __GNUC__
-# define NODE_GNUC_AT_LEAST(major, minor, patch) \- (NODE_MAKE_VERSION(major, minor, patch) <= \- NODE_MAKE_VERSION(__GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__))+#define NODE_GNUC_AT_LEAST(major, minor, patch) \+ (NODE_MAKE_VERSION(major, minor, patch) <= \+ NODE_MAKE_VERSION(__GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__))
#else
-# define NODE_GNUC_AT_LEAST(major, minor, patch) (0)+#define NODE_GNUC_AT_LEAST(major, minor, patch) (0)
#endif

and

diff --git a/src/node_version.h b/src/node_version.h
index 3c4eb70771..0691a2bcca 100644
--- a/src/node_version.h+++ b/src/node_version.h@@ -41,35 +41,34 @@--#define NODE_VERSION_AT_LEAST(major, minor, patch) \- (( (major) < NODE_MAJOR_VERSION) \- || ((major) == NODE_MAJOR_VERSION && (minor) < NODE_MINOR_VERSION) \- || ((major) == NODE_MAJOR_VERSION && \- (minor) == NODE_MINOR_VERSION && (patch) <= NODE_PATCH_VERSION))+#define NODE_VERSION_AT_LEAST(major, minor, patch) \+ (((major) < NODE_MAJOR_VERSION) || \+ ((major) == NODE_MAJOR_VERSION && (minor) < NODE_MINOR_VERSION) || \+ ((major) == NODE_MAJOR_VERSION && (minor) == NODE_MINOR_VERSION && \+ (patch) <= NODE_PATCH_VERSION))

TBH I find the new styles better but those are all tricky macros so I guess we can just leave them as a future exercise for whoever touches those lines to decide whether they want to use NOLINT or clang-format off comments to resolve the conflict between the linter and clang-format.

joyeecheung · 2018-08-01T10:21:38Z

Also, ideally we can get rid of most of cpplint (at least the parts that can be formatted) once we manage to enforce clang-format in our landing flow or a git-clang-format check in a pre-push/pre-commit hook. It should be easier if we don't even need to think about style nits at all and just let clang-format enforce a style (even if it sometimes produces less good-looking code due to its limitation).

joyeecheung · 2018-08-01T14:14:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16118/

(Not that the CI actually covers the changes here..)

devsnek · 2018-08-01T14:57:56Z

i think people should probably take a look at #21998... it has some odd changes.

targos · 2018-08-01T15:05:53Z

@devsnek can you make recommendations here about what you would like to be changed?

See https://clang.llvm.org/docs/ClangFormatStyleOptions.html for the list of options and what they do.

targos · 2018-08-01T15:07:30Z

+AllowAllParametersOfDeclarationOnNextLine: true
+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: Inline


Maybe this should be set to Empty? @devsnek

I think this is fine, since Inline implies Empty and we do do this for class field accessors quite a bit.

targos · 2018-08-01T15:09:34Z

+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: Inline
+AllowShortIfStatementsOnASingleLine: true


It looks like our current style doesn't do that. @devsnek

joyeecheung · 2018-08-01T15:21:15Z

About the styles: currently there are a lot of flexibility in our practice, I think we may be better off landing this first and then come back tweaking the entries in .clang-format later. The config here currently is mostly based on Google's style which is also used by V8. We can have our variants for sure but since we have not been consistent with our styles anyway, it's probably better if we look at them one by one later instead of trying to pay the debt and reflect on our code styles in this PR. After all this does not actually changes the source, we may continue the discussion in #21998

TimothyGu · 2018-08-01T17:21:33Z

@joyeecheung Thanks for working on the linter-formatter conflicts. I'd be more than fine with marking the two remaining cases with NOLINT – I prefer the newer style as well personally.

joyeecheung · 2018-08-03T09:40:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16167/

joyeecheung · 2018-08-03T11:40:57Z

The last CI was green. Landed in Landed in 0da144f, thanks!

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. PR-URL: #21997 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added 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. labels Jul 27, 2018

joyeecheung mentioned this pull request Jul 27, 2018
[WIP] tools: use clang-format to format C++ code #16122
Closed
2 tasks

targos requested review from addaleax and bnoordhuis July 27, 2018 09:29

joyeecheung mentioned this pull request Jul 27, 2018
[DO NOT LAND] src: run clang-format on LINT_CPP_FILES #21998
Closed
4 tasks

BridgeAR reviewed Jul 27, 2018
View reviewed changes

addaleax approved these changes Jul 27, 2018
View reviewed changes

jasnell approved these changes Jul 27, 2018
View reviewed changes

devsnek approved these changes Jul 27, 2018
View reviewed changes

TimothyGu reviewed Jul 28, 2018
View reviewed changes

joyeecheung force-pushed the clang-format-diff branch from 5481fef to fa7f447Compare August 1, 2018 09:30

targos approved these changes Aug 1, 2018
View reviewed changes

targos reviewed Aug 1, 2018
View reviewed changes

joyeecheung closed this Aug 3, 2018

rvagg mentioned this pull request Aug 13, 2018
Release proposal: v10.9.0 #22295
Merged

joyeecheung mentioned this pull request Feb 6, 2019
[Internal] clang-format the whole repo #25955
Closed

RaisinTen mentioned this pull request Apr 11, 2022
build: run clang-format on CI #42681
Merged

Uh oh!

Uh oh!

Conversation

joyeecheung commented Jul 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jul 27, 2018

Uh oh!

targos commented Jul 27, 2018

Uh oh!

joyeecheung commented Jul 27, 2018

Uh oh!

joyeecheung commented Jul 27, 2018

Uh oh!

joyeecheung commented Jul 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheungJul 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleaxJul 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheungJul 27, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Jul 27, 2018

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Aug 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Aug 1, 2018

Uh oh!

joyeecheung commented Aug 1, 2018

Uh oh!

devsnek commented Aug 1, 2018

Uh oh!

targos commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Aug 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Aug 1, 2018

Uh oh!

joyeecheung commented Aug 3, 2018

Uh oh!

joyeecheung commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

joyeecheung commented Jul 27, 2018•
edited
Loading

joyeecheungJul 27, 2018•
edited
Loading

addaleaxJul 27, 2018•
edited
Loading

joyeecheungJul 27, 2018•
edited
Loading

joyeecheung commented Aug 1, 2018•
edited
Loading

joyeecheung commented Aug 1, 2018•
edited
Loading