[clang-tools-extra] [clang-tidy] Enable container-contains fix-its in macro arguments (PR #232196)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 14 23:33:22 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Andre Sun (and2049)
<details>
<summary>Changes</summary>
Allow `readability-container-contains` to fix explicit boolean comparisons spelled within a single macro argument when the parameter's uses preserve expression grouping or constitute the entire macro body. Keep diagnostics without fixes for unsupported macro uses, including ambiguous forwarding cases.
Closes #<!-- -->226833
Assisted by: Claude Code
---
Full diff: https://github.com/llvm/llvm-project/pull/232196.diff
4 Files Affected:
- (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp (+113-14)
- (modified) clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h (+5)
- (modified) clang-tools-extra/docs/ReleaseNotes.md (+5)
- (modified) clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp (+164)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
index 701fab500f646a..97c9c3c2457518 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.cpp
@@ -7,14 +7,78 @@
//===----------------------------------------------------------------------===//
#include "ContainerContainsCheck.h"
+#include "../utils/ASTUtils.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
using namespace clang::ast_matchers;
namespace clang::tidy::readability {
+// Returns true if the macro body uses the parameter expanded at `ParamUseLoc`
+// only parenthesized or as the entire body, so every expansion parses alike.
+static bool isParameterUsedOnlyParenthesized(SourceLocation ParamUseLoc,
+ Preprocessor &PP,
+ const SourceManager &SM) {
+ const StringRef MacroName =
+ Lexer::getImmediateMacroName(ParamUseLoc, SM, PP.getLangOpts());
+ if (MacroName.empty())
+ return false;
+ const MacroInfo *MI =
+ PP.getMacroDefinitionAtLoc(PP.getIdentifierInfo(MacroName),
+ SM.getExpansionLoc(ParamUseLoc))
+ .getMacroInfo();
+ if (!MI)
+ return false;
+
+ const ArrayRef<Token> Tokens = MI->tokens();
+ const SourceLocation ParamDefLoc = SM.getSpellingLoc(ParamUseLoc);
+ const auto *ParamTok = llvm::find_if(Tokens, [ParamDefLoc](const Token &Tok) {
+ return Tok.getLocation() == ParamDefLoc;
+ });
+ if (ParamTok == Tokens.end())
+ return false;
+ if (Tokens.size() == 1)
+ return true;
+
+ const IdentifierInfo *Param = ParamTok->getIdentifierInfo();
+ for (size_t I = 0, N = Tokens.size(); I < N; ++I) {
+ if (Tokens[I].getIdentifierInfo() != Param)
+ continue;
+ if (I == 0 || I + 1 == N || Tokens[I - 1].isNot(tok::l_paren) ||
+ Tokens[I + 1].isNot(tok::r_paren))
+ return false;
+ // Parentheses that may complete a call or a macro invocation, including
+ // one reached through a parameter or another expansion, do not group.
+ if (I >= 2 &&
+ Tokens[I - 2].isOneOf(tok::identifier, tok::r_paren, tok::r_square,
+ tok::r_brace, tok::greater, tok::greatergreater))
+ return false;
+ }
+ return true;
+}
+
+// Returns true if every macro argument that `Loc` is spelled through satisfies
+// `isParameterUsedOnlyParenthesized`.
+static bool isSpelledThroughParenthesizedMacroArguments(
+ SourceLocation Loc, Preprocessor &PP, const SourceManager &SM) {
+ for (; Loc.isMacroID(); Loc = SM.getImmediateMacroCallerLoc(Loc)) {
+ SourceLocation ParamUseLoc;
+ if (SM.isMacroArgExpansion(Loc, &ParamUseLoc) &&
+ !isParameterUsedOnlyParenthesized(ParamUseLoc, PP, SM))
+ return false;
+ }
+ return true;
+}
+
+void ContainerContainsCheck::registerPPCallbacks(
+ const SourceManager &SM, Preprocessor *PP,
+ Preprocessor * /*ModuleExpanderPP*/) {
+ this->PP = PP;
+}
+
void ContainerContainsCheck::registerMatchers(MatchFinder *Finder) {
const auto Literal0 = integerLiteral(equals(0));
const auto Literal1 = integerLiteral(equals(1));
@@ -93,37 +157,72 @@ void ContainerContainsCheck::check(const MatchFinder::MatchResult &Result) {
const auto *Comparison = Negated ? NegativeComparison : PositiveComparison;
const StringRef ContainsFunName =
Result.Nodes.getNodeAs<CXXMethodDecl>("contains_fun")->getName();
- const Expr *SearchExpr = Call->getArg(0)->IgnoreParenImpCasts();
// Diagnose the issue.
const auto Diag = diag(Call->getExprLoc(), "use '%0' to check for membership")
<< ContainsFunName;
- // Don't fix it if it's in a macro invocation. Leave fixing it to the user.
- const SourceLocation FuncCallLoc = Comparison->getEndLoc();
- if (!FuncCallLoc.isValid() || FuncCallLoc.isMacroID())
+ const SourceLocation ComparisonEndLoc = Comparison->getEndLoc();
+ if (ComparisonEndLoc.isInvalid())
return;
+ const SourceManager &SM = *Result.SourceManager;
+ const LangOptions &LangOpts = Result.Context->getLangOpts();
+
+ const bool HasPrefix = Comparison->getBeginLoc() != Call->getBeginLoc();
+ CharSourceRange PrefixRange = CharSourceRange::getCharRange(
+ Comparison->getBeginLoc(), Call->getBeginLoc());
+ CharSourceRange FunctionNameRange =
+ CharSourceRange::getTokenRange(Call->getExprLoc());
+ CharSourceRange ArgumentsAndSuffixRange = CharSourceRange::getTokenRange(
+ Call->getArg(0)->getBeginLoc(), ComparisonEndLoc);
+ SourceLocation NegationLoc = Call->getBeginLoc();
+
+ if (ComparisonEndLoc.isMacroID()) {
+ if (!PP || isa<ImplicitCastExpr>(Comparison) ||
+ !Comparison->getType()->isBooleanType() ||
+ !utils::rangeIsEntirelyWithinMacroArgument(Comparison->getSourceRange(),
+ &SM) ||
+ !isSpelledThroughParenthesizedMacroArguments(Comparison->getBeginLoc(),
+ *PP, SM) ||
+ !isSpelledThroughParenthesizedMacroArguments(ComparisonEndLoc, *PP, SM))
+ return;
+
+ const CharSourceRange CallRange = Lexer::makeFileCharRange(
+ CharSourceRange::getTokenRange(Call->getSourceRange()), SM, LangOpts);
+ if (HasPrefix)
+ PrefixRange = Lexer::makeFileCharRange(PrefixRange, SM, LangOpts);
+ FunctionNameRange =
+ Lexer::makeFileCharRange(FunctionNameRange, SM, LangOpts);
+ ArgumentsAndSuffixRange =
+ Lexer::makeFileCharRange(ArgumentsAndSuffixRange, SM, LangOpts);
+ if (CallRange.isInvalid() || PrefixRange.isInvalid() ||
+ FunctionNameRange.isInvalid() || ArgumentsAndSuffixRange.isInvalid())
+ return;
+ NegationLoc = CallRange.getBegin();
+ }
+
+ bool Invalid = false;
const StringRef SearchExprText = Lexer::getSourceText(
- CharSourceRange::getTokenRange(SearchExpr->getSourceRange()),
- *Result.SourceManager, Result.Context->getLangOpts());
+ CharSourceRange::getTokenRange(Call->getArg(0)->getSourceRange()), SM,
+ LangOpts, &Invalid);
+ if (Invalid)
+ return;
// Remove everything before the function call.
- Diag << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
- Comparison->getBeginLoc(), Call->getBeginLoc()));
+ if (HasPrefix)
+ Diag << FixItHint::CreateRemoval(PrefixRange);
// Rename the function to `contains`.
- Diag << FixItHint::CreateReplacement(Call->getExprLoc(), ContainsFunName);
+ Diag << FixItHint::CreateReplacement(FunctionNameRange, ContainsFunName);
// Replace arguments and everything after the function call.
- Diag << FixItHint::CreateReplacement(
- CharSourceRange::getTokenRange(Call->getArg(0)->getBeginLoc(),
- Comparison->getEndLoc()),
- (SearchExprText + ")").str());
+ Diag << FixItHint::CreateReplacement(ArgumentsAndSuffixRange,
+ (SearchExprText + ")").str());
// Add negation if necessary.
if (Negated)
- Diag << FixItHint::CreateInsertion(Call->getBeginLoc(), "!");
+ Diag << FixItHint::CreateInsertion(NegationLoc, "!");
}
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
index 8d043910fc2bac..5ecba315aec074 100644
--- a/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/ContainerContainsCheck.h
@@ -23,6 +23,8 @@ class ContainerContainsCheck : public ClangTidyCheck {
public:
ContainerContainsCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}
+ void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+ Preprocessor *ModuleExpanderPP) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) final;
void check(const ast_matchers::MatchFinder::MatchResult &Result) final;
bool isLanguageVersionSupported(const LangOptions &LO) const final {
@@ -31,6 +33,9 @@ class ContainerContainsCheck : public ClangTidyCheck {
std::optional<TraversalKind> getCheckTraversalKind() const override {
return TK_IgnoreUnlessSpelledInSource;
}
+
+private:
+ Preprocessor *PP = nullptr;
};
} // namespace clang::tidy::readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 69f3ecee49926b..2372f433d417a2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -330,6 +330,11 @@ infrastructure are described first, followed by tool-specific sections.
avoid offering fix-its for string literals originating from macro
expansions.
+- Improved {doc}`readability-container-contains
+ <clang-tidy/checks/readability/container-contains>` check by offering fix-its
+ for explicit comparisons spelled entirely within a macro argument that the
+ macro body only uses parenthesized.
+
- Improved {doc}`readability-convert-member-functions-to-static
<clang-tidy/checks/readability/convert-member-functions-to-static>` check by
fixing a crash when checking a const-qualified method declared with the
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
index 0cecd936027a53..0dda8843b3b40f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/container-contains.cpp
@@ -213,6 +213,170 @@ int testMacroExpansion(std::unordered_set<int> &MySet) {
return 0;
}
+// Explicit comparisons spelled entirely within a single macro argument.
+bool testMacroArgument(std::set<int> &MySet, int Key) {
+#define ONCE(E) ((E))
+ // CHECK-FIXES: #define ONCE(E) ((E))
+ bool A = ONCE(MySet.find(Key) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool A = ONCE(MySet.contains(Key));
+ bool B = ONCE(MySet.find(Key) == MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool B = ONCE(!MySet.contains(Key));
+ bool C = ONCE(MySet.count(Key) != 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool C = ONCE(MySet.contains(Key));
+ bool D = ONCE(0 == MySet.count(Key));
+ // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool D = ONCE(!MySet.contains(Key));
+#undef ONCE
+
+#define REPEAT(E) ((E) && (E) && (E))
+ // CHECK-FIXES: #define REPEAT(E) ((E) && (E) && (E))
+ bool E = REPEAT(MySet.find(Key) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool E = REPEAT(MySet.contains(Key));
+ bool F = REPEAT(MySet.find(Key) == MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool F = REPEAT(!MySet.contains(Key));
+#undef REPEAT
+
+#define ID(E) E
+ bool L = ID(MySet.find(Key) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool L = ID(MySet.contains(Key));
+#undef ID
+
+ // No fix when a use of the parameter is not parenthesized.
+#define TWICE(E) E && E
+ // CHECK-FIXES: #define TWICE(E) E && E
+ bool G = TWICE(MySet.find(Key) == MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool G = TWICE(MySet.find(Key) == MySet.end());
+#undef TWICE
+
+ // No fix for an implicitly converted `count` inside a macro argument.
+#define MIXED(E) (E && E == 2)
+ // CHECK-FIXES: #define MIXED(E) (E && E == 2)
+ bool H = MIXED(MySet.count(Key));
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool H = MIXED(MySet.count(Key));
+#undef MIXED
+
+ // Operands spelled through other macros.
+#define MY_SET MySet
+#define ONCE(E) ((E))
+ bool I = ONCE(MY_SET.find(Key) != MY_SET.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool I = ONCE(MY_SET.contains(Key));
+#undef MY_SET
+#define KEY Key
+ bool K = ONCE(MySet.find(KEY) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool K = ONCE(MySet.contains(KEY));
+#undef KEY
+#define METHOD find
+ bool M = ONCE(MySet.METHOD(Key) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool M = ONCE(MySet.contains(Key));
+#undef METHOD
+#define PAREN_KEY (Key)
+ bool N = ONCE(MySet.find(PAREN_KEY) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool N = ONCE(MySet.contains(PAREN_KEY));
+#undef PAREN_KEY
+ bool O = ONCE(MySet.find((Key)) != MySet.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool O = ONCE(MySet.contains((Key)));
+
+ // No fix when the comparison mixes argument and macro body tokens.
+#define IS_FOUND(E) (E != MySet.end())
+ // CHECK-FIXES: #define IS_FOUND(E) (E != MySet.end())
+ bool J = IS_FOUND(MySet.find(Key));
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: bool J = IS_FOUND(MySet.find(Key));
+#undef IS_FOUND
+#undef ONCE
+ return A && B && C && D && E && F && G && H && I && J && K && L && M && N && O;
+}
+
+struct ConstexprContainer {
+ constexpr const int *find(int X) const { return X == 1 ? &Value : nullptr; }
+ constexpr const int *end() const { return nullptr; }
+ constexpr unsigned count(int X) const { return X == 1; }
+ constexpr bool contains(int X) const { return X == 1; }
+ int Value = 1;
+};
+
+constexpr ConstexprContainer CC;
+#define REPEAT(E) ((E) && (E) && (E))
+#define TWICE(E) E && E
+#define MIXED(E) (E && E == 2)
+#define MIX_COMPARISON(E) ((E) && (E + 1))
+static_assert(REPEAT(CC.find(1) != CC.end()), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(REPEAT(CC.contains(1)), "");
+static_assert(TWICE(CC.find(2) == CC.end()), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(TWICE(CC.find(2) == CC.end()), "");
+static_assert(!MIXED(CC.count(1)), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(!MIXED(CC.count(1)), "");
+static_assert(!MIX_COMPARISON(CC.count(1) != 0), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:34: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(!MIX_COMPARISON(CC.count(1) != 0), "");
+#undef REPEAT
+#undef TWICE
+#undef MIXED
+#undef MIX_COMPARISON
+
+// No fix when a use may be the argument of a call or macro invocation.
+#define BAD(E) (E + 1)
+#define ID(X) X
+#define FORWARD(E) ((E) && BAD(E))
+static_assert(!FORWARD(CC.count(1) != 0), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(!FORWARD(CC.count(1) != 0), "");
+#define VIA_PARAMETER(F, E) ((E) && F(E))
+static_assert(!VIA_PARAMETER(BAD, CC.count(1) != 0), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:38: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(!VIA_PARAMETER(BAD, CC.count(1) != 0), "");
+#define VIA_EXPANSION(E) ((E) && ID(BAD)(E))
+static_assert(!VIA_EXPANSION(CC.count(1) != 0), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:33: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(!VIA_EXPANSION(CC.count(1) != 0), "");
+constexpr bool identity(bool B) { return B; }
+#define CALL(E) ((E) && identity(E))
+static_assert(CALL(CC.count(1) != 0), "");
+// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: use 'contains' to check for membership [readability-container-contains]
+// CHECK-FIXES: static_assert(CALL(CC.count(1) != 0), "");
+#undef BAD
+#undef ID
+#undef FORWARD
+#undef VIA_PARAMETER
+#undef VIA_EXPANSION
+#undef CALL
+
+struct NonBoolIterator {
+ int operator!=(NonBoolIterator) const;
+};
+
+struct NonBoolComparisonContainer {
+ NonBoolIterator find(int) const;
+ NonBoolIterator end() const;
+ bool contains(int) const;
+};
+
+// No fix for a non-`bool` comparison inside a macro argument.
+int testMacroArgumentNonBoolComparison(NonBoolComparisonContainer &C, int Key) {
+#define ONCE(E) ((E))
+ int R = ONCE(C.find(Key) != C.end());
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: use 'contains' to check for membership [readability-container-contains]
+ // CHECK-FIXES: int R = ONCE(C.find(Key) != C.end());
+#undef ONCE
+ return R;
+}
+
// The following map has the same interface as `std::map`.
template <class Key, class T>
struct CustomMap {
``````````
</details>
https://github.com/llvm/llvm-project/pull/232196
More information about the cfe-commits
mailing list