[clang][diagnostics] add '-Wundef-true' warning option #128265
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
|
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
isuckatcs
requested review from
5chmidti,
EugeneZelenko,
NagyDonat,
PiotrZSL and
Xazax-hun
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-tidy Author: None (isuckatcs) ChangesIn C++, The following snippet returns int main() {
#if true
return 1;
#else
return 0;
#endif
}The check identifies such cases, when Full diff: https://github.com/llvm/llvm-project/pull/128265.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index c5f0b5b28418f..6d993669f2303 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -85,6 +85,7 @@
#include "TerminatingContinueCheck.h"
#include "ThrowKeywordMissingCheck.h"
#include "TooSmallLoopVariableCheck.h"
+#include "TrueMacroCheck.h"
#include "UncheckedOptionalAccessCheck.h"
#include "UndefinedMemoryManipulationCheck.h"
#include "UndelegatedConstructorCheck.h"
@@ -247,6 +248,7 @@ class BugproneModule : public ClangTidyModule {
"bugprone-throw-keyword-missing");
CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
"bugprone-too-small-loop-variable");
+ CheckFactories.registerCheck<TrueMacroCheck>("bugprone-true-macro");
CheckFactories.registerCheck<UncheckedOptionalAccessCheck>(
"bugprone-unchecked-optional-access");
CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index e8309c68b7fca..e479c3c137809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -86,6 +86,7 @@ add_clang_library(clangTidyBugproneModule STATIC
TerminatingContinueCheck.cpp
ThrowKeywordMissingCheck.cpp
TooSmallLoopVariableCheck.cpp
+ TrueMacroCheck.cpp
UncheckedOptionalAccessCheck.cpp
UndefinedMemoryManipulationCheck.cpp
UndelegatedConstructorCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/TrueMacroCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/TrueMacroCheck.cpp
new file mode 100644
index 0000000000000..4b1375db34039
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/TrueMacroCheck.cpp
@@ -0,0 +1,123 @@
+//===--- TrueMacroCheck.cpp - clang-tidy ----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TrueMacroCheck.h"
+#include "clang/Lex/MacroInfo.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+namespace {
+
+class MacroCallback : public PPCallbacks {
+ static constexpr const char *TrueMacroSpelling = "true";
+
+public:
+ MacroCallback(TrueMacroCheck *Check, Preprocessor *PP)
+ : Check(Check), PP(PP) {}
+ void MacroDefined(const Token &MacroNameTok,
+ const MacroDirective *MD) override {
+ if (TrueDefined)
+ return;
+
+ const MacroInfo *MI = MD->getMacroInfo();
+ for (const Token &Tok : MI->tokens()) {
+ if (PP->getSpelling(Tok) == TrueMacroSpelling)
+ emitDiagnostics(Tok.getLocation(),
+ {Tok.getLocation(), Tok.getEndLoc()});
+ }
+
+ if (PP->getSpelling(MacroNameTok) == TrueMacroSpelling)
+ TrueDefined = true;
+ }
+
+ virtual void MacroUndefined(const Token &MacroNameTok,
+ const MacroDefinition &MD,
+ const MacroDirective *Undef) override {
+ if (PP->getSpelling(MacroNameTok) == TrueMacroSpelling)
+ TrueDefined = false;
+ }
+
+ virtual void If(SourceLocation Loc, SourceRange ConditionRange,
+ ConditionValueKind ConditionValue) override {
+ StringRef Condition =
+ Lexer::getSourceText(CharSourceRange::getTokenRange(ConditionRange),
+ PP->getSourceManager(), PP->getLangOpts());
+
+ if (!TrueDefined && Condition == TrueMacroSpelling) {
+ emitDiagnostics(ConditionRange.getBegin(), ConditionRange);
+ return;
+ }
+
+ for (auto &&Identifier : identifiersInCondition(Condition)) {
+ if (!TrueDefined && Identifier == TrueMacroSpelling) {
+ emitDiagnostics(Loc, {}, true);
+ break;
+ }
+ }
+ }
+
+private:
+ void emitDiagnostics(SourceLocation Loc, SourceRange ReplaceRange,
+ bool InCondition = false) {
+ DiagnosticBuilder Builder =
+ Check->diag(Loc, "in C 'true'%select{| in the condition}0 is treated "
+ "as an undefined "
+ "macro and evaluates to a falsy value; "
+ "consider replacing it with '1'")
+ << InCondition;
+
+ if (!InCondition)
+ Builder << FixItHint::CreateReplacement(ReplaceRange, "1");
+ }
+
+ std::vector<StringRef> identifiersInCondition(StringRef Condition) {
+ const static auto Start = [](char C) {
+ return C == '_' || ('a' <= C && C <= 'z') || ('A' <= C && C <= 'Z');
+ };
+
+ const static auto Continue = [](char C) {
+ return ('0' <= C && C <= '9') || Start(C);
+ };
+
+ std::vector<StringRef> Identifiers;
+ const char *Str = nullptr;
+
+ for (size_t I = 0; I < Condition.size(); ++I) {
+ char C = Condition[I];
+
+ if (!Str && Start(C)) {
+ Str = Condition.begin() + I;
+ } else if (Str && !Continue(C)) {
+ Identifiers.emplace_back(Str, Condition.begin() + I - Str);
+ Str = nullptr;
+ }
+ }
+
+ if (Str)
+ Identifiers.emplace_back(Str, Condition.end() - Str);
+
+ return Identifiers;
+ }
+
+ bool TrueDefined = false;
+
+ TrueMacroCheck *Check;
+ Preprocessor *PP;
+};
+} // namespace
+
+void TrueMacroCheck::registerPPCallbacks(const SourceManager &SM,
+ Preprocessor *PP,
+ Preprocessor *ModuleExpanderPP) {
+ PP->addPPCallbacks(std::make_unique<MacroCallback>(this, PP));
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/TrueMacroCheck.h b/clang-tools-extra/clang-tidy/bugprone/TrueMacroCheck.h
new file mode 100644
index 0000000000000..24116dfcbeb4e
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/TrueMacroCheck.h
@@ -0,0 +1,34 @@
+//===--- TrueMacroCheck.h - clang-tidy --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TRUEMACROCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TRUEMACROCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// This check gives a warning if `true` is used in the preprocessor when not
+/// defined in C.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/true-macro.html
+class TrueMacroCheck : public ClangTidyCheck {
+public:
+ TrueMacroCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+ Preprocessor *ModuleExpanderPP) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return !LangOpts.CPlusPlus;
+ }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_TRUEMACROCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6b8fe22242417..51683093e5d23 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -91,6 +91,11 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
+- New :doc:`bugprone-true-macro
+ <clang-tidy/checks/bugprone/true-macro>` check.
+
+ This check gives a warning if ``true`` is used in the preprocessor when not defined in C.
+
New check aliases
^^^^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/true-macro.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/true-macro.rst
new file mode 100644
index 0000000000000..89ec804130b37
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/true-macro.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - bugprone-true-macro
+
+bugprone-true-macro
+===================
+
+In C++, ``true`` is considered a keyword by the preprocessor so an ``#if true`` enters the true branch,
+while in C, ``true`` is not treated as a special keyword by the preprocessor, so the false branch is entered.
+
+The following snippet returns ``1`` in C++, but ``0`` in C.
+
+.. code-block:: c++
+
+ int main() {
+ #if true
+ return 1;
+ #else
+ return 0;
+ #endif
+ }
+
+The check identifies such cases, when ``true`` is used without being defined first and also offers fix-its in
+some cases.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 7b9b905ef7671..2940e8ee7248a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -153,6 +153,7 @@ Clang-Tidy Checks
:doc:`bugprone-terminating-continue <bugprone/terminating-continue>`, "Yes"
:doc:`bugprone-throw-keyword-missing <bugprone/throw-keyword-missing>`,
:doc:`bugprone-too-small-loop-variable <bugprone/too-small-loop-variable>`,
+ :doc:`bugprone-true-macro <bugprone/true-macro>`, "Yes"
:doc:`bugprone-unchecked-optional-access <bugprone/unchecked-optional-access>`,
:doc:`bugprone-undefined-memory-manipulation <bugprone/undefined-memory-manipulation>`,
:doc:`bugprone-undelegated-constructor <bugprone/undelegated-constructor>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/true-macro-defintion.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/true-macro-defintion.c
new file mode 100644
index 0000000000000..060f42c4aee0f
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/true-macro-defintion.c
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy -std=c99-or-later %s bugprone-true-macro %t
+
+#define FOO true
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: in C 'true' is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+// CHECK-FIXES: 1
+
+#if true
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: in C 'true' is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+// CHECK-FIXES: 1
+#endif
+
+#if false || true
+// CHECK-MESSAGES: :[[@LINE-1]]:2: warning: in C 'true' in the condition is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+#endif
+
+#define true 1
+
+#define FOO true
+
+#if true
+#endif
+
+#if false || true
+#endif
+
+#undef true
+
+#define FOO true
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: in C 'true' is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+// CHECK-FIXES: 1
+
+#if true
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: in C 'true' is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+// CHECK-FIXES: 1
+#endif
+
+#if false || true
+// CHECK-MESSAGES: :[[@LINE-1]]:2: warning: in C 'true' in the condition is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+#endif
+
+#define true true
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: in C 'true' is treated as an undefined macro and evaluates to a falsy value; consider replacing it with '1' [bugprone-true-macro]
+// CHECK-FIXES: 1
|
clang-tools-extra/docs/clang-tidy/checks/bugprone/true-macro.rst
Outdated
Show resolved
Hide resolved
Contributor
|
FYI C23 introduced the |
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
46af06b to
6b6573f
Compare
steakhal
reviewed
Feb 23, 2025
clang-tools-extra/test/clang-tidy/checkers/bugprone/true-macro-defintion.c
Outdated
Show resolved
Hide resolved
PiotrZSL
reviewed
Feb 23, 2025
Member
There was a problem hiding this comment.
Some comments:
- Wrong category, maybe better would be 'portability'
- To short check name, maybe better would be portability-undefined-bool-literal or something similar.
- What about 'false' ?
- This issue is detected by -Wundef compiler warning in both clang and gcc.
Consider:
- Not adding check if -Wundef is sufficient
- Create check that forbids usage of true/false in processor as an non-portable thing (could be in portability category)
- Extracting 'true' case from -Wundef into separate warning like -Wundef-true
clang-tools-extra/test/clang-tidy/checkers/bugprone/true-macro-defintion.c
Outdated
Show resolved
Hide resolved
Member
Author
In both cases if
This is a nice catch, I didn't know about it 😅. IIUC, I can imagine cases when the macro is expected to be defined in a compiler argument, e.g.:
I like both of these approaches, though I prefer the 2. one because even if we extract |
Collaborator
Option 3 seems best to me. This check is much better suited to being done in the compiler rather than after the fact in a checker, and indeed we are already warning in the preprocessor. Splitting out a sub-warning |
Collaborator
That's true, but somewhat irrelevant, especially if we enable Clang-tidy uses clang to process the source file, so any warning implemented in clang is available to all clang-tidy users regardless of whether they compile with clang or GCC or something else. So if we implement this in clang, then any clang user gets the warning, and any clang-tidy user gets the warning too (even if they compile with GCC). But if we implement it in clang-tidy, then clang users who don't use clang-tidy only get the warning if they enable |
Member
Author
A fair point indeed. I'll proceed with option 3 then. |
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
6b6573f to
794e45b
Compare
llvmbot
added
clang
Member
Author
|
As per the discussion above, I move the check from |
isuckatcs
changed the title
true is used as a preprocessor keyword in C
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
794e45b to
3a528ed
Compare
zygoloid
reviewed
Mar 1, 2025
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
3a528ed to
7f7d4a5
Compare
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
7f7d4a5 to
6f08928
Compare
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
6f08928 to
dc4d302
Compare
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
dc4d302 to
628e549
Compare
Collaborator
|
Oh, we should also have a release note I think |
isuckatcs
force-pushed
the
bugprone-true-macro
branch
from
628e549 to
3f8633a
Compare
Member
Author
|
Added an entry to the release notes. |
zygoloid
approved these changes
Mar 2, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/13861 Here is the relevant piece of the build log for the reference |
Contributor
|
This found a bug in chromium, https://crbug.com/403123830 Thanks for this cool warning! :) |
Member
Author
|
🥰🥰🥰 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 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.
In C++,
trueis considered a keyword by the preprocessor so an#if trueenters the true branch,while in C,
trueis not treated as a special keyword by the preprocessor, so the false branch is entered.The following snippet returns
1in C++, but0in C.The check identifies such cases, when
trueis used without being defined first and also offers fix-its insome cases.