diff --git a/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp index 63649150b17e3..b6dde10fcdfce 100644 --- a/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp @@ -11,6 +11,9 @@ #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" #include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "llvm/Support/raw_ostream.h" +#include using namespace clang::ast_matchers; @@ -35,10 +38,59 @@ AST_MATCHER_P(Stmt, nextStmt, ast_matchers::internal::Matcher, return InnerMatcher.matches(**I, Finder, Builder); } + +static bool returnsBoolLiteral(const ReturnStmt *Ret, bool Value) { + if (!Ret || !Ret->getRetValue()) + return false; + + const auto *BoolLit = + dyn_cast(Ret->getRetValue()->IgnoreImplicit()); + return BoolLit && BoolLit->getValue() == Value; +} + +static StringRef getExprText(const Expr *E, const SourceManager &SM, + const LangOptions &LangOpts) { + return Lexer::getSourceText( + CharSourceRange::getTokenRange(E->getSourceRange()), SM, LangOpts); +} + +static SourceLocation getStmtEndIncludingSemicolon(const Stmt &S, + const SourceManager &SM, + const LangOptions &LangOpts) { + SourceLocation End = S.getEndLoc(); + if (std::optional Next = Lexer::findNextToken(End, SM, LangOpts)) { + if (Next->is(tok::semi)) + return Next->getEndLoc(); + } + return End; +} + +struct FixItInfo { + llvm::StringRef Algorithm; + std::string Replacement; +}; + } // namespace namespace tidy::readability { +UseAnyOfAllOfCheck::UseAnyOfAllOfCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + Inserter(Options.getLocalOrGlobal("IncludeStyle", + utils::IncludeSorter::IS_LLVM), + areDiagsSelfContained()) {} + +void UseAnyOfAllOfCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "IncludeStyle", Inserter.getStyle()); +} + +void UseAnyOfAllOfCheck::registerPPCallbacks(const SourceManager &SM, + Preprocessor *PP, + Preprocessor *ModuleExpanderPP) { + Inserter.registerPreprocessor(PP); +} + void UseAnyOfAllOfCheck::registerMatchers(MatchFinder *Finder) { auto Returns = [](bool V) { return returnStmt(hasReturnValue(cxxBoolLiteral(equals(V)))); @@ -84,20 +136,177 @@ static bool isViableLoop(const CXXForRangeStmt &S, ASTContext &Context) { }); } +static const IfStmt *getSingleIfInLoopBody(const CXXForRangeStmt &Loop) { + const Stmt *Body = Loop.getBody(); + if (const auto *If = dyn_cast(Body)) + return If; + + const auto *Compound = dyn_cast(Body); + if (!Compound || Compound->size() != 1) + return nullptr; + + return dyn_cast(Compound->body_front()); +} + +static std::optional +createFixItForLoop(const CXXForRangeStmt &Loop, const ReturnStmt &FinalReturn, + bool IfMustReturnTrue, const SourceManager &SM, + const LangOptions &LangOpts, bool UseRanges) { + if (Loop.getBeginLoc().isMacroID() || FinalReturn.getBeginLoc().isMacroID()) + return std::nullopt; + + const IfStmt *If = getSingleIfInLoopBody(Loop); + if (!If || If->getElse() || If->getInit() || If->getConditionVariable()) + return std::nullopt; + + const ReturnStmt *IfReturn = nullptr; + if (const auto *ThenCompound = dyn_cast(If->getThen())) { + if (ThenCompound->size() != 1) + return std::nullopt; + IfReturn = dyn_cast(ThenCompound->body_front()); + } else { + IfReturn = dyn_cast(If->getThen()); + } + + if (!returnsBoolLiteral(IfReturn, IfMustReturnTrue)) + return std::nullopt; + + if (!returnsBoolLiteral(&FinalReturn, !IfMustReturnTrue)) + return std::nullopt; + + const Expr *Container = Loop.getRangeInit(); + const VarDecl *LoopVar = Loop.getLoopVariable(); + const Expr *PredicateExpr = If->getCond(); + llvm::StringRef Algorithm = "any_of"; + + if (!IfMustReturnTrue) { + if (const auto *Negated = dyn_cast( + PredicateExpr->IgnoreParenImpCasts())) { + if (Negated->getOpcode() == UO_LNot) { + Algorithm = "all_of"; + PredicateExpr = Negated->getSubExpr(); + } else { + Algorithm = "none_of"; + } + } else { + Algorithm = "none_of"; + } + } + + const StringRef ContainerText = getExprText(Container, SM, LangOpts); + const StringRef PredicateText = getExprText(PredicateExpr, SM, LangOpts); + const StringRef LoopVarName = LoopVar->getName(); + + if (ContainerText.empty() || PredicateText.empty() || LoopVarName.empty()) + return std::nullopt; + + std::string LoopVarType; + const SourceLocation TypeStart = LoopVar->getBeginLoc(); + const SourceLocation NameStart = LoopVar->getLocation(); + if (TypeStart.isValid() && NameStart.isValid()) { + LoopVarType = + Lexer::getSourceText(CharSourceRange::getCharRange(TypeStart, NameStart), + SM, LangOpts) + .rtrim() + .str(); + } + if (LoopVarType.empty()) + LoopVarType = LoopVar->getType().getAsString(); + + const StringRef Separator = + (!LoopVarType.empty() && + (LoopVarType.back() == '&' || LoopVarType.back() == '*')) + ? "" + : " "; + + std::string Replacement; + llvm::raw_string_ostream OS(Replacement); + OS << "return std"; + if (UseRanges) { + OS << "::ranges::" << Algorithm << "(" << ContainerText << ", [" + << "](" << LoopVarType << Separator << LoopVarName << ") { return " + << PredicateText << "; });"; + } else { + OS << "::" << Algorithm << "(std::begin(" << ContainerText + << "), std::end(" << ContainerText << "), [" + << "](" << LoopVarType << Separator << LoopVarName << ") { return " + << PredicateText << "; });"; + } + OS.flush(); + + return FixItInfo{Algorithm, Replacement}; +} + void UseAnyOfAllOfCheck::check(const MatchFinder::MatchResult &Result) { - if (const auto *S = Result.Nodes.getNodeAs("any_of_loop")) { - if (!isViableLoop(*S, *Result.Context)) + const auto *FinalReturn = Result.Nodes.getNodeAs("final_return"); + if (!FinalReturn) + return; + + const bool UseRanges = getLangOpts().CPlusPlus20; + + if (const auto *Loop = Result.Nodes.getNodeAs("any_of_loop")) { + if (!isViableLoop(*Loop, *Result.Context)) + return; + + if (std::optional Fix = + createFixItForLoop(*Loop, *FinalReturn, true, *Result.SourceManager, + Result.Context->getLangOpts(), UseRanges)) { + auto Diag = + diag(Loop->getForLoc(), "replace loop by 'std%select{|::ranges}0::%1()'") + << UseRanges << Fix->Algorithm; + const SourceLocation ReplaceEnd = getStmtEndIncludingSemicolon( + *FinalReturn, *Result.SourceManager, Result.Context->getLangOpts()); + Diag << FixItHint::CreateReplacement( + CharSourceRange::getCharRange(Loop->getBeginLoc(), ReplaceEnd), + Fix->Replacement); + if (auto IncludeFixIt = Inserter.createIncludeInsertion( + Result.SourceManager->getFileID(Loop->getBeginLoc()), + "")) + Diag << *IncludeFixIt; + if (!UseRanges) { + if (auto IteratorIncludeFixIt = Inserter.createIncludeInsertion( + Result.SourceManager->getFileID(Loop->getBeginLoc()), + "")) + Diag << *IteratorIncludeFixIt; + } + return; + } + + diag(Loop->getForLoc(), "replace loop by 'std%select{|::ranges}0::any_of()'") + << UseRanges; + return; + } + + if (const auto *Loop = Result.Nodes.getNodeAs("all_of_loop")) { + if (!isViableLoop(*Loop, *Result.Context)) return; - diag(S->getForLoc(), "replace loop by 'std%select{|::ranges}0::any_of()'") - << getLangOpts().CPlusPlus20; - } else if (const auto *S = - Result.Nodes.getNodeAs("all_of_loop")) { - if (!isViableLoop(*S, *Result.Context)) + if (std::optional Fix = + createFixItForLoop(*Loop, *FinalReturn, false, *Result.SourceManager, + Result.Context->getLangOpts(), UseRanges)) { + auto Diag = + diag(Loop->getForLoc(), "replace loop by 'std%select{|::ranges}0::%1()'") + << UseRanges << Fix->Algorithm; + const SourceLocation ReplaceEnd = getStmtEndIncludingSemicolon( + *FinalReturn, *Result.SourceManager, Result.Context->getLangOpts()); + Diag << FixItHint::CreateReplacement( + CharSourceRange::getCharRange(Loop->getBeginLoc(), ReplaceEnd), + Fix->Replacement); + if (auto IncludeFixIt = Inserter.createIncludeInsertion( + Result.SourceManager->getFileID(Loop->getBeginLoc()), + "")) + Diag << *IncludeFixIt; + if (!UseRanges) { + if (auto IteratorIncludeFixIt = Inserter.createIncludeInsertion( + Result.SourceManager->getFileID(Loop->getBeginLoc()), + "")) + Diag << *IteratorIncludeFixIt; + } return; + } - diag(S->getForLoc(), "replace loop by 'std%select{|::ranges}0::all_of()'") - << getLangOpts().CPlusPlus20; + diag(Loop->getForLoc(), "replace loop by 'std%select{|::ranges}0::all_of()'") + << UseRanges; } } diff --git a/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h index 32983e48450f1..cd621ee75d403 100644 --- a/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h +++ b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h @@ -21,14 +21,20 @@ namespace clang::tidy::readability { /// https://clang.llvm.org/extra/clang-tidy/checks/readability/use-anyofallof.html class UseAnyOfAllOfCheck : public ClangTidyCheck { public: - using ClangTidyCheck::ClangTidyCheck; + UseAnyOfAllOfCheck(StringRef Name, ClangTidyContext *Context); bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { return LangOpts.CPlusPlus11; } void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + +private: + utils::IncludeInserter Inserter; }; } // namespace clang::tidy::readability diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 68bab88146241..cc177449c02b5 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -242,6 +242,10 @@ Changes in existing checks now uses separate note diagnostics for each uninitialized enumerator, making it easier to see which specific enumerators need explicit initialization. +- Improved :doc:`readability-use-anyofallof + ` check by adding fix-it hints + for simple loop forms and support for ``std::none_of`` suggestions. + - Improved :doc:`readability-non-const-parameter ` check by avoiding false positives on parameters used in dependent expressions (e.g. inside generic diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability/use-anyofallof.rst b/clang-tools-extra/docs/clang-tidy/checks/readability/use-anyofallof.rst index cfbc551bf07c0..00af908735174 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/readability/use-anyofallof.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/readability/use-anyofallof.rst @@ -4,8 +4,9 @@ readability-use-anyofallof ========================== Finds range-based for loops that can be replaced by a call to -``std::any_of`` or ``std::all_of``. In C++20 mode, suggests -``std::ranges::any_of`` or ``std::ranges::all_of``. +``std::any_of``, ``std::all_of``, or ``std::none_of``. In C++20 mode, +suggests ``std::ranges::any_of``, ``std::ranges::all_of``, or +``std::ranges::none_of``. Example: diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/use-anyofallof.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/use-anyofallof.cpp index 7f8f16488d37a..b77013221b154 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/use-anyofallof.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/use-anyofallof.cpp @@ -1,5 +1,4 @@ // RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t -- -- -fexceptions -// FIXME: Fix the checker to work in C++20 mode. bool good_any_of() { int v[] = {1, 2, 3}; @@ -8,6 +7,7 @@ bool good_any_of() { if (i) return true; return false; + // CHECK-FIXES: return std::any_of(std::begin(v), std::end(v), [](int i) { return i; }); } bool cond(int i); @@ -176,9 +176,20 @@ bool bad_any_of7() { bool good_all_of() { int v[] = {1, 2, 3}; - // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::all_of()' [readability-use-anyofallof] + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::none_of()' [readability-use-anyofallof] for (int i : v) if (i) return false; return true; + // CHECK-FIXES: return std::none_of(std::begin(v), std::end(v), [](int i) { return i; }); +} + +bool good_all_of_negated_condition() { + int v[] = {1, 2, 3}; + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::all_of()' [readability-use-anyofallof] + for (int i : v) + if (!cond(i)) + return false; + return true; + // CHECK-FIXES: return std::all_of(std::begin(v), std::end(v), [](int i) { return cond(i); }); }