build: upgrade clang-format to v18 #53957
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
RedYetiDev
added
build
There was a problem hiding this comment.
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. |
RedYetiDev
changed the title
|
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? |
RedYetiDev
changed the title
|
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! |
|
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? |
| @@ -9,11 +9,11 @@ AlignEscapedNewlines: Right | |||
| AlignOperands: true | |||
| AlignTrailingComments: true | |||
| AllowAllParametersOfDeclarationOnNextLine: true | |||
| AllowShortBlocksOnASingleLine: false | |||
There was a problem hiding this comment.
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.
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.
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.
I think an empty package json is more confusing
There was a problem hiding this comment.
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.
@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. |
RedYetiDev
added
the
commit-queue-squash
RedYetiDev
removed
c++
Fixes #50157
This PR updates the Clang formatter from the unmaintained
clang-formatto @magic-akari's amazing@wasm-fmt/clang-formatlibrary.The old formatter was unmaintained and vastly out of date.