-
Notifications
You must be signed in to change notification settings - Fork 29k
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
build: upgrade clang-format to v18 #53957
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
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 is my honor that you are using @wasm-fmt/clang-format.
I believe that some stylistic differences may arise from the version of LLVM in use.
The current version of @wasm-fmt/clang-format is based on LLVM 16.0.6.
Should you have specific requirements regarding the LLVM version, I am prepared to release the version that meets your needs.
Oh! I thought it was v19. Nevertheless, it's newer than angular and actively maintained. No need to prepare anything special, this is only a draft, and I'm not a core collaborator... I'm curious to see the feedback on this, as I'm hoping that an update of the formatter (this) (along with the linter in another PR) will help keep the CPP code clean and organized for the future. The stylistic issues are a result of both the linter and LLVM, so I'm hoping that if the linter is updated, at least one of them will be resolved. I'm going to do more testing to see if they are compatible with each other out of the box, but if not I can always configure the linter to work with the formatter. |
The second commit is not acceptable. We've been using a clang-format option that only formats the modified lines to avoid a huge change like this one. |
So, we need to support range formats, right? |
I've undone the file changes to make review easier, but that doesn't mean this is ready for review. |
I'm not an expert, but I think clang supports them in https://github.com/llvm/llvm-project/blob/710dab6e18ad9ed22c2529b9125d7b8813165ede/clang/tools/clang-format/ClangFormat.cpp#L234-L274, does your package have a config that allows them? |
Okay, I've switched it to a fork which is basically a copy of angular's, except updated to v18. If the WASM one supports the needs, I'm happy to switch it back, this change is (hopefully) only temporary. |
Okay, this fork (suprisingly) works well. It only lints the modified files and lines. I'd love to use the WASM version if it gains that support, because who doesn't love WASM, but (at least for now) this fork works! (Sorry for dragging you around @magic-akari) |
Also, |
Not now, but I am very willing to support them. |
Great! If your package supports them, that'd be lovely, but I'd hold off on modifying it for a bit, because this change still needs to be reviewed by the core collaborators. If this does land, and then your package fits the needs, I'd push for moving |
fwiw I started working on a fork as well (https://github.com/targos/clang-format/tree/test-build). I just didn't have much time to continue. I think it's important to have an approach where the builds can be trusted. That's why I investigated doing them on GitHub actions. |
Regarding GitHub actions, I came across https://github.com/muttleyxd/clang-tools-static-binaries, which uploads copies of them. Although that's a step for the future IMO. |
Sweet! All tests are passing :-) |
Starting from version 17.0.6, @wasm-fmt/clang-format supports range formatting. |
Great! I'll try to incorporate it into this PR soon, but I'm quite busy for the rest of this month (sorry!) Thank you so so much for your development and help to incorporate it, it's truly amazing. |
Hey everyone, I’m a bit busy for the next week or two, I know this was unfortunate time to open this PR, but I’ll undraft when I get a chance to complete it. thanks! |
Starting from version 18.1.6, you can effortlessly swap out angular's clang-format for a drop-in replacement with @wasm-fmt/clang-format. |
Thanks! I'm still out of town, but I'll check it out in a few days! |
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.
Since I don't have approval access, I can only give a LGTM. 👍
All tests pass, lint error is unrelated to this PR and was patched in a different PR. |
Thanks for your feedback, @magic-akari. @anonrig care to re-review? |
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.
Review notes
@@ -9,11 +9,11 @@ AlignEscapedNewlines: Right | |||
AlignOperands: true | |||
AlignTrailingComments: true | |||
AllowAllParametersOfDeclarationOnNextLine: true | |||
AllowShortBlocksOnASingleLine: false |
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.
Review note: This was changed, as AllowShortBlocksOnASingleLine
is a Never
/Empty
/Always
value.
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortblocksonasingleline
AllowShortCaseLabelsOnASingleLine: false | ||
AllowShortFunctionsOnASingleLine: Inline | ||
AllowShortIfStatementsOnASingleLine: true | ||
AllowShortLoopsOnASingleLine: 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.
Review note: This was changed, as the behavior of AllowShortLoopsOnASingleLine
was changed since this was true
. In order for formatting to remain the same as it currently is, AllowShortLoopsOnASingleLine
should be false
.
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortloopsonasingleline
Changes LGTM. @nodejs/security-wg can you have a look? |
@targos you merged |
No objections from the security team in a week, WDYT? |
"name": "node-core-clang-format", | ||
"version": "1.0.0", | ||
"description": "Formatting C++ files for Node.js core", | ||
"license": "MIT", |
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.
Why this was removed?
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.
Because this isn't a package, so it doesn't make sense to have a package name, it may confuse users.
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 an empty package json is more confusing
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 just think the name, version, and license are misleading, as there isn't any source code here.
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.
@marco-ippolito I've re-added the description, do you want me to add the rest?
I don't know how I did this merge. In any case, it was unintentional, sorry. |
Fixes #50157
This PR updates the Clang formatter from the unmaintained
clang-format
to @magic-akari's amazing@wasm-fmt/clang-format
library.The old formatter was unmaintained and vastly out of date.