Conversation
StephanDollberg
requested review from
Copilot,
dotnwat,
rockwotj and
travisdowns
rockwotj
left a comment
rockwotj
left a comment
There was a problem hiding this comment.
Don't we need to clang-format?
rockwotj
dismissed
their stale review
actually need to trigger lint-cpp CI check
There was a problem hiding this comment.
Pull request overview
This PR updates the repo’s Bazel-managed LLVM/Clang toolchain to make LLVM/Clang 22.1.0 the default (“current”) toolchain, removing the prior “current”/“next” toolchain entries and associated Bazel configs.
Changes:
- Promote LLVM/Clang 22.1.0 to the sole “current” toolchain definition in
MODULE.bazel. - Update
MODULE.bazel.lockto reflect the new toolchain repo specs/digests for@current_llvm_toolchain. - Simplify
.bazelrcby removing thecommon:clang-21/common:clang-22extra toolchain configs now that@current_llvm_toolchainis the default.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
MODULE.bazel |
Removes old 20.x/21.x compiler entries and sets “current” to LLVM/Clang 22.1.0 tarballs/shas. |
MODULE.bazel.lock |
Regenerates/updates bzlmod lock data so @current_llvm_toolchain resolves to LLVM/Clang 22.1.0. |
.bazelrc |
Drops now-redundant clang-21/clang-22 config blocks; continues to use @current_llvm_toolchain by default. |
We also need to format and add to .git-blame-ignore-refs or whatever the file is. Also consider backporting (just the clang-format stuff) to reduce conflicts with backports |
|
Yep good point. |
| std::string{schema_def.data(), schema_def.size()})); | ||
| !res.has_value()) { | ||
| if ( | ||
| auto res = sr_client->create_schema( |
There was a problem hiding this comment.
This is just odd, isn't it. Let me see whether one can disable.
There was a problem hiding this comment.
it seems kinda unfortunate that upgrading clang-format causes so much churn. are they "bugs" or just defaults changing and we can tweek our config? either way, no big deal.
There was a problem hiding this comment.
They are config. But it seems for this specifically it was inconsistent before so you get diffs either way. I don't care which way we go.
I'll leave default unless I hear otherwise.
There was a problem hiding this comment.
i find previous style
one cherry picked example:
- old style it is very easy to visually scan where if expression ends and where the if block starts
- new style: good luck since all lines are aligned to the same depth so you have to scan line ends rather than line starts
more readable
There was a problem hiding this comment.
seems for this specifically it was inconsistent before
got it so like formatting "bugs" so yeh it makes sense then to accept all the churn?
There was a problem hiding this comment.
btw our .clang-format files has AlignAfterOpenBracket: AlwaysBreak but it seems it applied only to function parameters in prev release
There was a problem hiding this comment.
there are more BreakAfterOpen* options to consider
Yes but they result in tens of thousands of line changes.
There was a problem hiding this comment.
Yes but they result in tens of thousands of line changes.
I mean if we can make this less painful in the future I think it's worth it.
There was a problem hiding this comment.
I don't really see what's painful about it right now but I also don't really see how we can avoid it.
The "problem" is that they are adding new flags for underspecified behaviour I think. So you can add explicit values for everything right now that still won't prevent future flags from causing changes.
There was a problem hiding this comment.
Agree with @StephanDollberg here, it looks like they switching from a heuristic (penalty) based breaking for these examples to more binary options which will always break after ( (for example) when it can't fit on one line. So we cannot get to zero churn.
I mean if we can make this less painful in the future I think it's worth it.
I don't think that's the case: we are just choosing between two different styles now, no reason to believe the other one will have less churn in the future. So I would vote for what we have now as the low churn option unless there is a very concrete reason we like the other way better.
How would this work btw? Backport the whole toolchain? |
|
Let's get a microbench on this. |
backport the toolchain as |
|
I guess clang-22 is already on 26.1 but we don't have a custom clang-format config which we could append to. We'd need to disable pre-commit hooks as well. Hmm. |
You could just change the clang-format target in bazel to |
Retry command for Build#82547please wait until all jobs are finished before running the slash command |
travisdowns
force-pushed
the
stephan/clang-22-default
branch
from
3ee1814 to
56b3b9a
Compare
|
@StephanDollberg I rebased this branch to get the CI to run again. |
Hmm, all feels very hacky. Ideally we'd have to:
|
StephanDollberg
force-pushed
the
stephan/clang-22-default
branch
2 times, most recently
from
4bf1bb2 to
3590b95
Compare
StephanDollberg
force-pushed
the
stephan/clang-22-default
branch
3 times, most recently
from
a8b1795 to
748272f
Compare
Simpler is to just bump the toolchain used for clang format only? Lines 134 to 137 in b75774d |
|
Ah interesting, that would be easier yes. |
|
/microbench |
|
@StephanDollberg wrote:
Thanks. "Looks fine" to me, all sorts of +/- but mostly modest. Overall trend not sure but definitely not a lot worse or anything like that. This one stuck out to me as odd, I may look at it further: |
Travis already made everything build.
```
external/toolchains_llvm++llvm+current_llvm_toolchain_llvm/include/clang-tidy/ClangTidyModuleRegistry.h:15:2: error: The ClangTidyModuleRegistry.h header is deprecated and will be removed in LLVM 24. All of the symbols it used to define have been moved into ClangTidyModule.h. [-Werror,-W#warnings]
15 | #warning The ClangTidyModuleRegistry.h header is deprecated and will be removed in LLVM 24. All of the symbols it used to define have been moved into ClangTidyModule.h.
| ^
1 error generated.
```
StephanDollberg
force-pushed
the
stephan/clang-22-default
branch
from
748272f to
e299de9
Compare
Our bazel clang-tidy wrapper is very weird and causes errors with clang-tidy-22: - We seem to be building compile flags manually in our bazel wrapper. This means we are somehow passing defines multiple times with different values (BAZEL_CURRENT_REPOSITORY ) - breaks now because of clang-diagnostic-macro-redefined. Disable that error. - We seem to run on headers directly which seems very odd to me and results in lots of unused variable warnings. clang-tidy-22 now errors about warnings in headers by default. Stop doing that.
Our weird bazel clang-tidy hackfest doesn't understand everything that is set in .bazelrc. Hence, it doesn't use the per-file-copt settings that we pass making it fail on errors that are ignored by that. To work around that remove -Werror which it gets passed down from our compiler opts. Instead drive -Werror completely from the .clang-tidy `WarningsAsErrors` options.
Multiple issues with the existing header regex:
Problem 1: YAML round-trip corruption. The Python wrapper (clang_tidy.py) loads
the .clang-tidy file with yaml.safe_load, modifies the Checks field, then
writes it back with yaml.safe_dump. The dump produces:
HeaderFilterRegex: ^(?!external/.*).*
Note: no quotes. LLVM's YAML parser sees !external as a YAML tag indicator,
corrupting the regex. This silently disables header filtering — no headers get
checked for WarningsAsErrors violations.
Problem 2: Path mismatch. Even if the regex survived YAML, it anchors at
^external/. But Bazel sandbox paths look like
bazel-out/k8-fastbuild/bin/external/protobuf+/... — they don't start with
external/, so the negative lookahead never triggers.
The new config:
- HeaderFilterRegex: '.*': check diagnostics from all headers -
- ExcludeHeaderFilterRegex: clang-tidy 18+ feature that excludes
matching headers:
- (^|/)external/ — matches external/ anywhere in the path (handles
Bazel's bazel-out/.../external/... paths)
- \.json\.hh$ — excludes Seastar-generated swagger headers
(auto-generated code we can't fix)
Neither value contains !, so they survive YAML round-tripping safely.
- src/v/utils/retry_chain_node.h — Fixed bugprone-use-after-move: format message once upfront instead of double-forwarding args. - src/v/cluster/partition_leaders_table.h — Fixed bugprone-use-after-move: capture f by reference instead of forwarding into a lambda inside a loop. - src/v/test_utils/randoms.h — Fixed bugprone-use-after-move: don't forward args in a loop, just pass them as lvalues.
StephanDollberg
force-pushed
the
stephan/clang-22-default
branch
from
e299de9 to
ef3fac5
Compare
|
|
||
| try: | ||
| extra_args = [ | ||
| "--extra-arg=-Wno-error", |
There was a problem hiding this comment.
I have a feeling there is a better way to fix this, but shouldn't block this PR
Travis already made everything build.
Backports Required
Release Notes