From add51e152aa6dc3aa7a51901a099b2ebe8cfe377 Mon Sep 17 00:00:00 2001 From: Matthias Gehre Date: Thu, 3 Oct 2019 00:39:46 +0200 Subject: [PATCH] [clang-tidy] add new check readability-use-anyofallof Summary: 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``. For now, no fixits are produced. Reviewers: aaron.ballman, alexfh, hokein Subscribers: mgorny, xazax.hun, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D77572 --- .../clang-tidy/readability/CMakeLists.txt | 2 + .../readability/ReadabilityTidyModule.cpp | 3 + .../clang-tidy/readability/UseAnyOfAllOfCheck.cpp | 109 ++++++++++++ .../clang-tidy/readability/UseAnyOfAllOfCheck.h | 41 +++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + clang-tools-extra/docs/clang-tidy/checks/list.rst | 1 + .../checks/readability-use-anyofallof.rst | 22 +++ .../checkers/readability-use-anyofallof-cpp20.cpp | 19 +++ .../checkers/readability-use-anyofallof.cpp | 183 +++++++++++++++++++++ 9 files changed, 386 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt index 5f674be..02003a7 100644 --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -42,8 +42,10 @@ add_clang_library(clangTidyReadabilityModule StringCompareCheck.cpp UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp + UseAnyOfAllOfCheck.cpp LINK_LIBS + clangAnalysis clangAST clangASTMatchers clangBasic diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp index 5ece15e..5ff5e20 100644 --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -45,6 +45,7 @@ #include "StringCompareCheck.h" #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" +#include "UseAnyOfAllOfCheck.h" namespace clang { namespace tidy { @@ -125,6 +126,8 @@ public: "readability-uniqueptr-delete-release"); CheckFactories.registerCheck( "readability-uppercase-literal-suffix"); + CheckFactories.registerCheck( + "readability-use-anyofallof"); } }; diff --git a/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp new file mode 100644 index 0000000..d3c002f --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.cpp @@ -0,0 +1,109 @@ +//===--- UseAnyOfAllOfCheck.cpp - clang-tidy-------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "UseAnyOfAllOfCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h" +#include "clang/Frontend/CompilerInstance.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace { +/// Matches a Stmt whose parent is a CompoundStmt, and which is directly +/// followed by a Stmt matching the inner matcher. +AST_MATCHER_P(Stmt, nextStmt, ast_matchers::internal::Matcher, + InnerMatcher) { + DynTypedNodeList Parents = Finder->getASTContext().getParents(Node); + if (Parents.size() != 1) + return false; + + auto *C = Parents[0].get(); + if (!C) + return false; + + const auto *I = llvm::find(C->body(), &Node); + assert(I != C->body_end() && "C is parent of Node"); + if (++I == C->body_end()) + return false; // Node is last statement. + + return InnerMatcher.matches(**I, Finder, Builder); +} +} // namespace + +namespace tidy { +namespace readability { + +void UseAnyOfAllOfCheck::registerMatchers(MatchFinder *Finder) { + auto returns = [](bool V) { + return returnStmt(hasReturnValue(cxxBoolLiteral(equals(V)))); + }; + + auto returnsButNotTrue = + returnStmt(hasReturnValue(unless(cxxBoolLiteral(equals(true))))); + auto returnsButNotFalse = + returnStmt(hasReturnValue(unless(cxxBoolLiteral(equals(false))))); + + Finder->addMatcher( + cxxForRangeStmt( + nextStmt(returns(false).bind("final_return")), + hasBody(allOf(hasDescendant(returns(true)), + unless(anyOf(hasDescendant(breakStmt()), + hasDescendant(gotoStmt()), + hasDescendant(returnsButNotTrue)))))) + .bind("any_of_loop"), + this); + + Finder->addMatcher( + cxxForRangeStmt( + nextStmt(returns(true).bind("final_return")), + hasBody(allOf(hasDescendant(returns(false)), + unless(anyOf(hasDescendant(breakStmt()), + hasDescendant(gotoStmt()), + hasDescendant(returnsButNotFalse)))))) + .bind("all_of_loop"), + this); +} + +static bool isViableLoop(const CXXForRangeStmt &S, ASTContext &Context) { + + ExprMutationAnalyzer Mutations(*S.getBody(), Context); + if (Mutations.isMutated(S.getLoopVariable())) + return false; + const auto Matches = + match(findAll(declRefExpr().bind("decl_ref")), *S.getBody(), Context); + + return llvm::none_of(Matches, [&Mutations](auto &DeclRef) { + // TODO: allow modifications of loop-local variables + return Mutations.isMutated( + DeclRef.template getNodeAs("decl_ref")->getDecl()); + }); +} + +void UseAnyOfAllOfCheck::check(const MatchFinder::MatchResult &Result) { + StringRef Ranges = getLangOpts().CPlusPlus2a ? "::ranges" : ""; + + if (const auto *S = Result.Nodes.getNodeAs("any_of_loop")) { + if (!isViableLoop(*S, *Result.Context)) + return; + + diag(S->getForLoc(), "replace loop by 'std%0::any_of()'") << Ranges; + } else if (const auto *S = + Result.Nodes.getNodeAs("all_of_loop")) { + if (!isViableLoop(*S, *Result.Context)) + return; + + diag(S->getForLoc(), "replace loop by 'std%0::all_of()'") << Ranges; + } +} + +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h new file mode 100644 index 0000000..c44ef2e --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UseAnyOfAllOfCheck.h @@ -0,0 +1,41 @@ +//===--- UseAnyOfAllOfCheck.h - clang-tidy-----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALGORITHMCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALGORITHMCHECK_H + +#include "../ClangTidy.h" +#include "../utils/IncludeInserter.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Finds ranged-based for loops that can be replaced by a call to std::any_of +/// or std::all_of. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-use-anyofallof.html +class UseAnyOfAllOfCheck : public ClangTidyCheck { +public: + using ClangTidyCheck::ClangTidyCheck; + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } + + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USEALGORITHMCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index e6583c1..8da24a9 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -152,6 +152,12 @@ New checks Finds calls to ``NSInvocation`` methods under ARC that don't have proper argument object lifetimes. +- New :doc:`readability-use-anyofallof + ` check. + + Finds range-based for loops that can be replaced by a call to ``std::any_of`` + or ``std::all_of``. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 6d5f8fc..3794aa5 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -299,6 +299,7 @@ Clang-Tidy Checks `readability-string-compare `_, "Yes" `readability-uniqueptr-delete-release `_, "Yes" `readability-uppercase-literal-suffix `_, "Yes" + `readability-use-anyofallof`_, "No" `zircon-temporary-objects `_, 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 new file mode 100644 index 0000000..f7bd9ff --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-use-anyofallof.rst @@ -0,0 +1,22 @@ +.. title:: clang-tidy - readability-use-anyofallof + +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``. + +Example: + +.. code-block:: c++ + + bool all_even(std::vector V) { + for (int I : V) { + if (I % 2) + return false; + } + return true; + // Replace loop by + // return std::ranges::all_of(V, [](int I) { return I % 2 == 0; }); + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp new file mode 100644 index 0000000..fdf98fd --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof-cpp20.cpp @@ -0,0 +1,19 @@ +// RUN: %check_clang_tidy -std=c++2a-or-later %s readability-use-anyofallof %t + +bool good_any_of() { + int v[] = {1, 2, 3}; + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::ranges::any_of()' + for (int i : v) + if (i) + return true; + return false; +} + +bool good_all_of() { + int v[] = {1, 2, 3}; + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::ranges::all_of()' + for (int i : v) + if (i) + return false; + return true; +} 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 new file mode 100644 index 0000000..5b19dd0 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-use-anyofallof.cpp @@ -0,0 +1,183 @@ +// RUN: %check_clang_tidy -std=c++14,c++17 %s readability-use-anyofallof %t + +bool good_any_of() { + int v[] = {1, 2, 3}; + // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: replace loop by 'std::any_of()' [readability-use-anyofallof] + for (int i : v) + if (i) + return true; + return false; +} + +bool cond(int i); + +bool good_any_of2() { + int v[] = {1, 2, 3}; + for (int i : v) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()' + int k = i / 2; + if (cond(k)) + return true; + } + return false; +} + +bool good_any_of3() { + int v[] = {1, 2, 3}; + for (int i : v) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()' + if (i == 3) + continue; + if (i) + return true; + } + + return false; +} + +bool good_any_of_use_external(int comp) { + int v[] = {1, 2, 3}; + for (int i : v) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()' + if (i == comp) + return true; + } + + return false; +} + +bool good_any_of_no_cond() { + int v[] = {1, 2, 3}; + for (int i : v) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()' + return true; // Not a real loop, but technically can become any_of. + } + + return false; +} + +bool good_any_of_local_modification() { + int v[] = {1, 2, 3}; + for (int i : v) { + int j = i; + j++; // FIXME: Any non-const use disables check. + if (j > 3) + return true; + } + + return false; +} + +bool good_any_of_throw() { + int v[] = {1, 2, 3}; + for (int i : v) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: replace loop by 'std::any_of()' + if (i > 3) + return true; + if (i == 42) + throw 0; + } + + return false; +} + +bool bad_any_of1() { + int v[] = {1, 2, 3}; + for (int i : v) { + if (i) + return false; // bad constant + } + return false; +} + +bool bad_any_of2() { + int v[] = {1, 2, 3}; + for (int i : v) + if (i) + return true; + + return true; // bad return +} + +bool bad_any_of3() { + int v[] = {1, 2, 3}; + for (int i : v) + if (i) + return true; + else + return i / 2; // bad return + + return false; +} + +bool bad_any_of_control_flow1() { + int v[] = {1, 2, 3}; + for (int i : v) { + break; // bad control flow + if (i) + return true; + } + + return false; +} + +bool bad_any_of_control_flow2() { + int v[] = {1, 2, 3}; + for (int i : v) { + goto end; // bad control flow + if (i) + return true; + } + + end: + return false; +} + +bool bad_any_of4() { + return false; // wrong order + + int v[] = {1, 2, 3}; + for (int i : v) { + if (i) + return true; + } +} + +bool bad_any_of5() { + int v[] = {1, 2, 3}; + int j = 0; + for (int i : v) { + j++; // modifications + if (i) + return true; + } + return false; +} + +bool bad_any_of6() { + int v[] = {1, 2, 3}; + for (int i : v) { + if (i) + return true; + } + int j = 0; // Statements between loop and return + j++; + return false; +} + +bool bad_any_of7() { + int v[] = {1, 2, 3}; + for (int i : v) { + i; // No 'return true' in body. + } + return false; +} + +bool good_all_of() { + 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 (i) + return false; + return true; +} -- 2.7.4