Update files and clang-tidy config to pass with clang-tidy-20 #4691
Conversation
|
Based on #4691 to get a better delta for .clang-tidy |
testing/file_test/autoupdate.cpp
Outdated
| @@ -294,7 +295,7 @@ auto FileTestAutoupdater::StartSplitFile() -> void { | |||
| AddCheckLines(stdout_, /*to_file_end=*/false); | |||
| } | |||
|
|
|||
| ++non_check_line_; | |||
There was a problem hiding this comment.
This doesn't really seem like an improvement to me... Could we disable this one too?
There was a problem hiding this comment.
To be sure you have context, this is bugprone-pointer-arithmetic-on-polymorphic-object. I'm hesitant to disable the bugprone warning due to what it's looking for, are you sure we don't want it?
There was a problem hiding this comment.
Thought of a better fix here, with final
There was a problem hiding this comment.
Oh, I see. It's about the polymorphic class!
Yeah, I think final is better here. I think std::advance would be just as bug prone as the ++. But the final is a pretty nice improvement.
| ASSERT_FALSE(simd_buffer.has_errors()); | ||
| EXPECT_THAT(simd_buffer.comments_size(), Eq(1)); | ||
| } | ||
| } | ||
|
|
||
| TEST_F(LexerTest, MultipleComments) { | ||
| constexpr llvm::StringLiteral Format = R"( |
There was a problem hiding this comment.
Specifically for StringLiteral, I think we should fix it to have a c_str() method. Would it make sense to disable the tidy until that lands in LLVM and we pull it back in?
I agree with you that this seems like a net negative change.
There was a problem hiding this comment.
To be sure, I was mainly trying to clarify the edits -- I think it's probably worth keeping on. In particular, the warning in testing/file_test/file_test_base.cpp seems like an improvement, and this spot seems more neutral to me. (the closest negative for me was in kind_pattern, since its use is distant from the forward declaration)
Noting the uncertainty of future changes to upstream, how about a TODO to switch instead of disabling? TBH I'd actually prefer for formatv to take llvm::StringLiteral, over .c_str
jonmeow
force-pushed
the
llvm-20-tidy
branch
from
0519501 to
2422de7
Compare
testing/file_test/autoupdate.cpp
Outdated
| @@ -294,7 +295,7 @@ auto FileTestAutoupdater::StartSplitFile() -> void { | |||
| AddCheckLines(stdout_, /*to_file_end=*/false); | |||
| } | |||
|
|
|||
| ++non_check_line_; | |||
There was a problem hiding this comment.
Oh, I see. It's about the polymorphic class!
Yeah, I think final is better here. I think std::advance would be just as bug prone as the ++. But the final is a pretty nice improvement.
| ASSERT_FALSE(simd_buffer.has_errors()); | ||
| EXPECT_THAT(simd_buffer.comments_size(), Eq(1)); | ||
| } | ||
| } | ||
|
|
||
| TEST_F(LexerTest, MultipleComments) { | ||
| constexpr llvm::StringLiteral Format = R"( |
Disables three new warnings because they lean more towards style conflicts than fixes. I've brought these up on #style.
Other than that, mostly fixing basic issues, and things that clang-tidy-20 seems to fire where clang-tiday-16 didn't. One particular curious case is
llvm::StringLiteral::data()uses, which are flagged as not strictly null-terminated; I'm switching toconst char*in those spots which matchesllvm::formatv's format argument, but feels worse.I'm removing
run_clang_tidy.pyhere because I'm observing it give fewer warnings thanbazel build --config=clang-tidy -k //toolchain/.... The latter matches how we enforce in GitHub actions (and also caches results, and suppresses output for files that have no issues), so I'm dropping the bespoke script.