cppcoreguidelines-macro-usage: Skip common macros which cannot be converted to constexpr
#80797
Conversation
Eliminate false positive when a macro makes use of built-in `__FILE__` or `__LINE__` in its tokens. There is no way to get this information from a `constexpr` and therefore suggesting a replacement in these cases is non-enforceable.
Eliminate false positive when a macro makes use stringifying expressions. There is no way to get this information from a `constexpr` and therefore suggesting a replacement in these cases is non-enforceable.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Sandy (bwrsandman) ChangesWith clang-tidy, The two cases here are the use of built-in macros such as Note to reviewers: Full diff: https://github.com/llvm/llvm-project/pull/80797.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 0cd4bf6fdfd87..d5b38ecd619f6 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -80,6 +80,20 @@ void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
const MacroInfo *Info = MD->getMacroInfo();
StringRef Message;
+ for (const auto &T : MD->getMacroInfo()->tokens()) {
+ if (T.is(tok::hash)) {
+ return;
+ }
+ if (T.is(tok::identifier)) {
+ StringRef IdentName = T.getIdentifierInfo()->getName();
+ if (IdentName == "__FILE__") {
+ return;
+ } else if (IdentName == "__LINE__") {
+ return;
+ }
+ }
+ }
+
if (llvm::all_of(Info->tokens(), std::mem_fn(&Token::isLiteral)))
Message = "macro '%0' used to declare a constant; consider using a "
"'constexpr' constant";
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 404aafb6b1c45..5b5fdd937cb3c 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
@@ -43,4 +43,10 @@
#define DLLEXPORTS __declspec(dllimport)
#endif
+int log(const char* file, unsigned int line, const char* message);
+#define LOG(message) log(__FILE__, __LINE__, (message));
+
+#define STRINGIFYIMPL(s) #s
+#define STRINGIFY(s) STRINGIFYIMPL(s)
+
#endif
|
|
I think Release Notes should be updated. |
| for (const auto &T : MD->getMacroInfo()->tokens()) { | ||
| if (T.is(tok::hash)) { | ||
| return; | ||
| } | ||
| if (T.is(tok::identifier)) { | ||
| StringRef IdentName = T.getIdentifierInfo()->getName(); | ||
| if (IdentName == "__FILE__") { | ||
| return; | ||
| } else if (IdentName == "__LINE__") { | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Won't work for:
#define MyLineAndFile __FILE__, __LINE__
#define logSomething(x) log(MyLineAndFile, x)
For me MacroExpands woud need to be used. And even for this scenario, would be good to add some configuration option regex and match to that.
There was a problem hiding this comment.
If I understand MacroExpands macro, it only gets invoked if the macro is used therefore it would need to be called somewhere for the check to trigger. Your code snippet will still error out with MacroDefined and MacroExpands won't ever be invoked.
Is there a way to use the post-processor inside of MacroDefined to expand the definition?
There was a problem hiding this comment.
Yes, probably easiest option would be simply to add config option like IgnoreMacrosThatUse, or something similar, make it regexp, and then add release note entry that "Macros that use directly an built-in macros are ignored". If you dont want to make that option public, it's fine, it can be some global static vector with strings in a check. Just to avoid bunch of ifs....
With clang-tidy,
cppcoreguidelines-macro-usageis very hard to enforce without setting regex whitelists.The reason for this is that many common practices are impossible to do outside of the precompiler.
The two cases here are the use of built-in macros such as
__FILE__and__LINE__and stringification.Note to reviewers:
This is my first contribution to this project so I'm going in a bit blind.
Please let me know of any helper functions I missed such as identifying the builtin macros or stringification.