-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: add make format-cpp
to run clang-format on C++ diffs
#21997
Conversation
Is it possible to force it to format all files in src (just to see the result)? |
@targos Sure, I can open another PR to show the diff. |
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
Outdated
BreakConstructorInitializers: BeforeColon | ||
BreakAfterJavaFieldAnnotations: false | ||
BreakStringLiterals: true | ||
ColumnLimit: 79 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should likely be 80 instead (we use that as the current limit).
AlignConsecutiveAssignments: false | ||
AlignConsecutiveDeclarations: false | ||
AlignEscapedNewlines: Right | ||
AlignOperands: true |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
SpacesInParentheses: false | ||
SpacesInSquareBrackets: false | ||
Standard: Auto | ||
TabWidth: 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Right below there is the option UseTab: Never
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.clang-format
Outdated
AlwaysBreakAfterReturnType: None | ||
AlwaysBreakBeforeMultilineStrings: false | ||
AlwaysBreakTemplateDeclarations: true | ||
BinPackArguments: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
.clang-format
Outdated
AlwaysBreakBeforeMultilineStrings: false | ||
AlwaysBreakTemplateDeclarations: true | ||
BinPackArguments: true | ||
BinPackParameters: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.clang-format
Outdated
ConstructorInitializerIndentWidth: 4 | ||
ContinuationIndentWidth: 4 | ||
Cpp11BracedListStyle: true | ||
DerivePointerAlignment: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, is this for the N-API C test files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@nodejs/build-files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
5481fef
to
fa7f447
Compare
@BridgeAR @addaleax @TimothyGu Thanks for the reviews, I've tweaked the 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
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 |
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). |
CI: https://ci.nodejs.org/job/node-test-pull-request/16118/ (Not that the CI actually covers the changes here..) |
i think people should probably take a look at #21998... it has some odd changes. |
@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. |
AllowAllParametersOfDeclarationOnNextLine: true | ||
AllowShortBlocksOnASingleLine: false | ||
AllowShortCaseLabelsOnASingleLine: false | ||
AllowShortFunctionsOnASingleLine: Inline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be set to Empty
? @devsnek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, since Inline
implies Empty
and we do do this for class field accessors quite a bit.
AllowShortBlocksOnASingleLine: false | ||
AllowShortCaseLabelsOnASingleLine: false | ||
AllowShortFunctionsOnASingleLine: Inline | ||
AllowShortIfStatementsOnASingleLine: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like our current style doesn't do that. @devsnek
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 |
@joyeecheung Thanks for working on the linter-formatter conflicts. I'd be more than fine with marking the two remaining cases with |
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>
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>
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 Makefilethat runs clang-format on the C++ diffs, and a
make format-cpp-build
to install clang-format fromnpm.
To format staged changes:
To format HEAD~1...HEAD (latest commit):
To format diff between master and current branch head (master...HEAD):
Most of the .clang-format file comes from running
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), orvcbuild test
(Windows) passes