-
Notifications
You must be signed in to change notification settings - Fork 16k
[clang-tidy] Allow type-generic builtins in pro-type-vararg check #178656
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
base: main
Are you sure you want to change the base?
[clang-tidy] Allow type-generic builtins in pro-type-vararg check #178656
Conversation
|
@PiotrZSL Could you please review this? This fixes a false positive for type generic builtins like |
|
@llvm/pr-subscribers-clang-tools-extra Author: puneeth_aditya_5656 (mugiwaraluffy56) ChangesSummaryAdd type generic builtins to the allowed variadics list in the
Root CauseThese builtins are declared as variadic ( TestAdded test cases in Fixes #17862 Full diff: https://github.com/llvm/llvm-project/pull/178656.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
index d5ff4af84b0b7..73cadfdf14614 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
@@ -56,6 +56,11 @@ static constexpr StringRef AllowedVariadics[] = {
"__builtin_nontemporal_store",
"__builtin_nontemporal_load",
"__builtin_ms_va_start",
+ // Type-generic builtins: declared variadic to accept any integer type.
+ "__builtin_clzg",
+ "__builtin_ctzg",
+ "__builtin_popcountg",
+ "__builtin_bswapg",
// clang-format on
};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
index 3f73d1de333f4..1a5a5fb7dd93c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
@@ -57,6 +57,12 @@ void ignoredBuiltinsTest(void *ptr) {
(void)__builtin_fpclassify(0, 0, 0, 0, 0, 0.f);
(void)__builtin_isinf_sign(0.f);
(void)__builtin_prefetch(nullptr);
+
+ // GH#178629: Type-generic builtins should not warn.
+ (void)__builtin_clzg(1u);
+ (void)__builtin_ctzg(1u);
+ (void)__builtin_popcountg(1u);
+ (void)__builtin_bswapg(1u);
}
// Some implementations of __builtin_va_list and __builtin_ms_va_list desugared
|
|
@llvm/pr-subscribers-clang-tidy Author: puneeth_aditya_5656 (mugiwaraluffy56) ChangesSummaryAdd type generic builtins to the allowed variadics list in the
Root CauseThese builtins are declared as variadic ( TestAdded test cases in Fixes #17862 Full diff: https://github.com/llvm/llvm-project/pull/178656.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
index d5ff4af84b0b7..73cadfdf14614 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
@@ -56,6 +56,11 @@ static constexpr StringRef AllowedVariadics[] = {
"__builtin_nontemporal_store",
"__builtin_nontemporal_load",
"__builtin_ms_va_start",
+ // Type-generic builtins: declared variadic to accept any integer type.
+ "__builtin_clzg",
+ "__builtin_ctzg",
+ "__builtin_popcountg",
+ "__builtin_bswapg",
// clang-format on
};
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
index 3f73d1de333f4..1a5a5fb7dd93c 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
@@ -57,6 +57,12 @@ void ignoredBuiltinsTest(void *ptr) {
(void)__builtin_fpclassify(0, 0, 0, 0, 0, 0.f);
(void)__builtin_isinf_sign(0.f);
(void)__builtin_prefetch(nullptr);
+
+ // GH#178629: Type-generic builtins should not warn.
+ (void)__builtin_clzg(1u);
+ (void)__builtin_ctzg(1u);
+ (void)__builtin_popcountg(1u);
+ (void)__builtin_bswapg(1u);
}
// Some implementations of __builtin_va_list and __builtin_ms_va_list desugared
|
zeyi2
left a comment
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.
Please add an entry in ReleaseNotes.rst :)
3455ca6 to
183e6c6
Compare
Added the ReleaseNotes entry. Thanks for the 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.
Overall LGTM, thank you!
But please wait for other reviewers :)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/pro-type-vararg.cpp
Show resolved
Hide resolved
localspook
left a comment
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.
Apart from the open comment, LGTM
| "__builtin_clzg", | ||
| "__builtin_ctzg", | ||
| "__builtin_popcountg", | ||
| "__builtin_bswapg", |
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.
Can we just check hasCustomTypechecking() instead of listing out a bunch of builtins here? The fact that these builtins are "variadic" is an clang implementation detail, and all the functions you're excluding here have that bit set.
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.
@efriedma-quic Great suggestion! Updated to check hasCustomTypechecking() instead of listing individual builtins. This is more maintainable and handles all builtins that use variadic declarations as an implementation detail.
183e6c6 to
5ce1602
Compare
|
✅ With the latest revision this PR passed the C/C++ code linter. |
5ce1602 to
00688cf
Compare
|
@efriedma-quic @localspook @zeyi2 @PiotrZSL @vbvictor can you review this |
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
Outdated
Show resolved
Hide resolved
00688cf to
3498a11
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Add __builtin_clzg, __builtin_ctzg, __builtin_popcountg, and __builtin_bswapg to the allowed variadics list. These type-generic builtins are declared as variadic to accept any integer type, but they are not C-style vararg functions and should not trigger the hicpp-vararg / cppcoreguidelines-pro-type-vararg warning. Fixes llvm#178629
3498a11 to
5227669
Compare
zeyi2
left a comment
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.
LGTM :)
Summary
Add type generic builtins to the allowed variadics list in the
cppcoreguidelines-pro-type-varargcheck (also used byhicpp-vararg):__builtin_clzg__builtin_ctzg__builtin_popcountg__builtin_bswapgRoot Cause
These builtins are declared as variadic (
int(...)) to accept any integer type viaCustomTypeChecking. However, they are not C style vararg functions , they take exactly one argument of a generic integer type.Test
Added test cases in
pro-type-vararg.cppto verify no warning is emitted.Fixes #178629