[clang-tidy] Do not lint on attribute macros #164806
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: wieDasDing (dingxiangfei2009) Changes
In short, we should not suggest code writers to rewrite this macro with #define FORMAT_STR(format_msg, first_idx) __attribute__((format(printf, format_msg, first_idx)))Full diff: https://github.com/llvm/llvm-project/pull/164806.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
index 766cae45f15b5..8370e51284867 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -82,6 +82,13 @@ void MacroUsageCheck::registerPPCallbacks(const SourceManager &SM,
void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
const MacroInfo *Info = MD->getMacroInfo();
StringRef Message;
+ bool PossiblyNotFunctionLike;
+ if (Info->getNumTokens() > 0) {
+ const Token &Tok = Info->getReplacementToken(0);
+ PossiblyNotFunctionLike = Tok.is(tok::kw___attribute);
+ } else {
+ PossiblyNotFunctionLike = false;
+ }
if (llvm::all_of(Info->tokens(), std::mem_fn(&Token::isLiteral)))
Message = "macro '%0' used to declare a constant; consider using a "
@@ -89,10 +96,10 @@ void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
// A variadic macro is function-like at the same time. Therefore variadic
// macros are checked first and will be excluded for the function-like
// diagnostic.
- else if (Info->isVariadic())
+ else if (Info->isVariadic() && !PossiblyNotFunctionLike)
Message = "variadic macro '%0' used; consider using a 'constexpr' "
"variadic template function";
- else if (Info->isFunctionLike())
+ else if (Info->isFunctionLike() && !PossiblyNotFunctionLike)
Message = "function-like macro '%0' used; consider a 'constexpr' template "
"function";
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/macro-usage.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/macro-usage.cpp
index 865ef9df1182e..f3eb8185878f2 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/macro-usage.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/macro-usage.cpp
@@ -47,4 +47,6 @@
#define DLLEXPORTS __declspec(dllimport)
#endif
+#define ATTRIBUTE_MACRO(...) __attribute__(__VA_ARGS__)
+
#endif
|
|
Worth keeping in mind that the rule implements the C++ Core Guidelines, which have a pretty wide ban on macros in general:
So depending on the interpretation It's possible to whitelist macros via the I do agree clang-tidy should not suggest replacing with a constexpr function in this case. EDIT: based on existing tests and seeing we support other macros, it appears the check is already more relaxed than what the rules say, so I'd be ok with this change. |
|
Please mention changes in Release Notes. |
EugeneZelenko
requested review from
HerrCai0907,
carlosgalvezp,
localspook and
vbvictor
I see that this rule is interesting. It could possibly make code harder to deal with feature variety in real world compilers. Macros are quite useful to provide a uniform access to compiler features. This rules do not seem to take this into calculation unfortunately... |
vbvictor
reviewed
Oct 24, 2025
| void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) { | ||
| const MacroInfo *Info = MD->getMacroInfo(); | ||
| StringRef Message; | ||
| bool PossiblyNotFunctionLike; |
There was a problem hiding this comment.
Could we invert logic of this variable and use PossiblyFunctionLike. Later instead of !PossiblyNotFunctionLike we should just write PossiblyFunctionLike which is easier and clearer.
There was a problem hiding this comment.
I have chosen a positive wording MacroBodyExpressionLike so that it is supposedly easier to reason about. Hopefully this is better.
Yep, the rule is quite strict. Typically this is handled with options to make it less strict. |
dingxiangfei2009
force-pushed
the
unlint-attribute-macros
branch
from
61b5340 to
9dd12cc
Compare
vbvictor
reviewed
Oct 27, 2025
| moved to the ``fuchsia`` module instead. The ``zircon`` module will be removed | ||
| in the 24th release. | ||
|
|
||
| - Improved :program:`clang-tidy`'s `cppcoreguidelines-macro-usage` check. |
There was a problem hiding this comment.
Please write release notes in section "Changes in existing checks" following alphabetical structure and established style there.
cppcoreguidelines-macro-usage lint incorrectly identifies these macros as candidates for rewrite into template arguments. There are no, variadic or not, equivalent to these macros using templated functions. Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
dingxiangfei2009
force-pushed
the
unlint-attribute-macros
branch
from
9dd12cc to
5f6f084
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/12322 Here is the relevant piece of the build log for the reference |
Lukacma
pushed a commit
to Lukacma/llvm-project
that referenced
this pull request
Oct 29, 2025
`cppcoreguidelines-macro-usage` lint incorrectly identifies these macros as candidates for rewrite into template arguments. There are no, variadic or not, equivalent to these macros using templated functions. In short, we should not suggest code writers to rewrite this macro with `constexpr` functions. ```c #define FORMAT_STR(format_msg, first_idx) __attribute__((format(printf, format_msg, first_idx))) ``` Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
aokblast
pushed a commit
to aokblast/llvm-project
that referenced
this pull request
Oct 30, 2025
`cppcoreguidelines-macro-usage` lint incorrectly identifies these macros as candidates for rewrite into template arguments. There are no, variadic or not, equivalent to these macros using templated functions. In short, we should not suggest code writers to rewrite this macro with `constexpr` functions. ```c #define FORMAT_STR(format_msg, first_idx) __attribute__((format(printf, format_msg, first_idx))) ``` Signed-off-by: Xiangfei Ding <dingxiangfei2009@protonmail.ch>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cppcoreguidelines-macro-usagelint incorrectly identifies these macros as candidates for rewrite into template arguments. There are no, variadic or not, equivalent to these macros using templated functions.In short, we should not suggest code writers to rewrite this macro with
constexprfunctions.