Merge commit '3ac2cf6b077c' from apple/main into swift/next

Author: git apple-llvm automerger

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

@@ -19,6 +19,8 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/WithColor.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cstring>
 #include <utility>
@@ -57,6 +59,7 @@ namespace modernize {
 
 static const char LoopNameArray[] = "forLoopArray";
 static const char LoopNameIterator[] = "forLoopIterator";
+static const char LoopNameReverseIterator[] = "forLoopReverseIterator";
 static const char LoopNamePseudoArray[] = "forLoopPseudoArray";
 static const char ConditionBoundName[] = "conditionBound";
 static const char ConditionVarName[] = "conditionVar";
@@ -68,7 +71,6 @@ static const char ConditionEndVarName[] = "conditionEndVar";
 static const char EndVarName[] = "endVar";
 static const char DerefByValueResultName[] = "derefByValueResult";
 static const char DerefByRefResultName[] = "derefByRefResult";
-
 // shared matchers
 static const TypeMatcher AnyType() { return anything(); }
 
@@ -150,10 +152,17 @@ StatementMatcher makeArrayLoopMatcher() {
 ///   - The iterator variables 'it', 'f', and 'h' are the same.
 ///   - The two containers on which 'begin' and 'end' are called are the same.
 ///   - If the end iterator variable 'g' is defined, it is the same as 'f'.
-StatementMatcher makeIteratorLoopMatcher() {
+StatementMatcher makeIteratorLoopMatcher(bool IsReverse) {
+
+  auto BeginNameMatcher = IsReverse ? hasAnyName("rbegin", "crbegin")
+                                    : hasAnyName("begin", "cbegin");
+
+  auto EndNameMatcher =
+      IsReverse ? hasAnyName("rend", "crend") : hasAnyName("end", "cend");
+
   StatementMatcher BeginCallMatcher =
       cxxMemberCallExpr(argumentCountIs(0),
-                        callee(cxxMethodDecl(hasAnyName("begin", "cbegin"))))
+                        callee(cxxMethodDecl(BeginNameMatcher)))
           .bind(BeginCallName);
 
   DeclarationMatcher InitDeclMatcher =
@@ -167,7 +176,7 @@ StatementMatcher makeIteratorLoopMatcher() {
       varDecl(hasInitializer(anything())).bind(EndVarName);
 
   StatementMatcher EndCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("end", "cend"))));
+      argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
 
   StatementMatcher IteratorBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
@@ -225,7 +234,7 @@ StatementMatcher makeIteratorLoopMatcher() {
                      hasArgument(
                          0, declRefExpr(to(varDecl(TestDerefReturnsByValue)
                                                .bind(IncrementVarName))))))))
-      .bind(LoopNameIterator);
+      .bind(IsReverse ? LoopNameReverseIterator : LoopNameIterator);
 }
 
 /// The matcher used for array-like containers (pseudoarrays).
@@ -325,7 +334,7 @@ StatementMatcher makePseudoArrayLoopMatcher() {
 /// the output parameter isArrow is set to indicate whether the initialization
 /// is called via . or ->.
 static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
-                                                bool *IsArrow) {
+                                                bool *IsArrow, bool IsReverse) {
   // FIXME: Maybe allow declaration/initialization outside of the for loop.
   const auto *TheCall =
       dyn_cast_or_null<CXXMemberCallExpr>(digThroughConstructors(Init));
@@ -336,9 +345,11 @@ static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
   if (!Member)
     return nullptr;
   StringRef Name = Member->getMemberDecl()->getName();
-  StringRef TargetName = IsBegin ? "begin" : "end";
-  StringRef ConstTargetName = IsBegin ? "cbegin" : "cend";
-  if (Name != TargetName && Name != ConstTargetName)
+  if (!Name.consume_back(IsBegin ? "begin" : "end"))
+    return nullptr;
+  if (IsReverse && !Name.consume_back("r"))
+    return nullptr;
+  if (!Name.empty() && !Name.equals("c"))
     return nullptr;
 
   const Expr *SourceExpr = Member->getBase();
@@ -356,18 +367,19 @@ static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
 /// must be a member.
 static const Expr *findContainer(ASTContext *Context, const Expr *BeginExpr,
                                  const Expr *EndExpr,
-                                 bool *ContainerNeedsDereference) {
+                                 bool *ContainerNeedsDereference,
+                                 bool IsReverse) {
   // Now that we know the loop variable and test expression, make sure they are
   // valid.
   bool BeginIsArrow = false;
   bool EndIsArrow = false;
-  const Expr *BeginContainerExpr =
-      getContainerFromBeginEndCall(BeginExpr, /*IsBegin=*/true, &BeginIsArrow);
+  const Expr *BeginContainerExpr = getContainerFromBeginEndCall(
+      BeginExpr, /*IsBegin=*/true, &BeginIsArrow, IsReverse);
   if (!BeginContainerExpr)
     return nullptr;
 
-  const Expr *EndContainerExpr =
-      getContainerFromBeginEndCall(EndExpr, /*IsBegin=*/false, &EndIsArrow);
+  const Expr *EndContainerExpr = getContainerFromBeginEndCall(
+      EndExpr, /*IsBegin=*/false, &EndIsArrow, IsReverse);
   // Disallow loops that try evil things like this (note the dot and arrow):
   //  for (IteratorType It = Obj.begin(), E = Obj->end(); It != E; ++It) { }
   if (!EndContainerExpr || BeginIsArrow != EndIsArrow ||
@@ -475,27 +487,59 @@ static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) {
 
 LoopConvertCheck::RangeDescriptor::RangeDescriptor()
     : ContainerNeedsDereference(false), DerefByConstRef(false),
-      DerefByValue(false) {}
+      DerefByValue(false), NeedsReverseCall(false) {}
 
 LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
       MaxCopySize(Options.get("MaxCopySize", 16ULL)),
       MinConfidence(Options.get("MinConfidence", Confidence::CL_Reasonable)),
-      NamingStyle(Options.get("NamingStyle", VariableNamer::NS_CamelCase)) {}
+      NamingStyle(Options.get("NamingStyle", VariableNamer::NS_CamelCase)),
+      Inserter(Options.getLocalOrGlobal("IncludeStyle",
+                                        utils::IncludeSorter::IS_LLVM)),
+      UseCxx20IfAvailable(Options.get("UseCxx20ReverseRanges", true)),
+      ReverseFunction(Options.get("MakeReverseRangeFunction", "")),
+      ReverseHeader(Options.get("MakeReverseRangeHeader", "")) {
+
+  if (ReverseFunction.empty() && !ReverseHeader.empty()) {
+    llvm::WithColor::warning()
+        << "modernize-loop-convert: 'MakeReverseRangeHeader' is set but "
+           "'MakeReverseRangeFunction' is not, disabling reverse loop "
+           "transformation\n";
+    UseReverseRanges = false;
+  } else if (ReverseFunction.empty()) {
+    UseReverseRanges = UseCxx20IfAvailable && getLangOpts().CPlusPlus20;
+  } else {
+    UseReverseRanges = true;
+  }
+}
 
 void LoopConvertCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
-  Options.store(Opts, "MaxCopySize", std::to_string(MaxCopySize));
+  Options.store(Opts, "MaxCopySize", MaxCopySize);
   Options.store(Opts, "MinConfidence", MinConfidence);
   Options.store(Opts, "NamingStyle", NamingStyle);
+  Options.store(Opts, "IncludeStyle", Inserter.getStyle());
+  Options.store(Opts, "UseCxx20ReverseRanges", UseCxx20IfAvailable);
+  Options.store(Opts, "MakeReverseRangeFunction", ReverseFunction);
+  Options.store(Opts, "MakeReverseRangeHeader", ReverseHeader);
+}
+
+void LoopConvertCheck::registerPPCallbacks(const SourceManager &SM,
+                                           Preprocessor *PP,
+                                           Preprocessor *ModuleExpanderPP) {
+  Inserter.registerPreprocessor(PP);
 }
 
 void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(traverse(ast_type_traits::TK_AsIs, makeArrayLoopMatcher()),
                      this);
   Finder->addMatcher(
-      traverse(ast_type_traits::TK_AsIs, makeIteratorLoopMatcher()), this);
+      traverse(ast_type_traits::TK_AsIs, makeIteratorLoopMatcher(false)), this);
   Finder->addMatcher(
       traverse(ast_type_traits::TK_AsIs, makePseudoArrayLoopMatcher()), this);
+  if (UseReverseRanges)
+    Finder->addMatcher(
+        traverse(ast_type_traits::TK_AsIs, makeIteratorLoopMatcher(true)),
+        this);
 }
 
 /// Given the range of a single declaration, such as:
@@ -646,13 +690,29 @@ void LoopConvertCheck::doConversion(
     }
   }
 
-  StringRef MaybeDereference = Descriptor.ContainerNeedsDereference ? "*" : "";
-  std::string TypeString = Type.getAsString(getLangOpts());
-  std::string Range = ("(" + TypeString + " " + VarName + " : " +
-                       MaybeDereference + Descriptor.ContainerString + ")")
-                          .str();
+  SmallString<128> Range;
+  llvm::raw_svector_ostream Output(Range);
+  Output << '(';
+  Type.print(Output, getLangOpts());
+  Output << ' ' << VarName << " : ";
+  if (Descriptor.NeedsReverseCall)
+    Output << getReverseFunction() << '(';
+  if (Descriptor.ContainerNeedsDereference)
+    Output << '*';
+  Output << Descriptor.ContainerString;
+  if (Descriptor.NeedsReverseCall)
+    Output << "))";
+  else
+    Output << ')';
   FixIts.push_back(FixItHint::CreateReplacement(
       CharSourceRange::getTokenRange(ParenRange), Range));
+
+  if (Descriptor.NeedsReverseCall && !getReverseHeader().empty()) {
+    if (Optional<FixItHint> Insertion = Inserter.createIncludeInsertion(
+            Context->getSourceManager().getFileID(Loop->getBeginLoc()),
+            getReverseHeader()))
+      FixIts.push_back(*Insertion);
+  }
   diag(Loop->getForLoc(), "use range-based for loop instead") << FixIts;
   TUInfo->getGeneratedDecls().insert(make_pair(Loop, VarName));
 }
@@ -762,8 +822,9 @@ void LoopConvertCheck::determineRangeDescriptor(
     const UsageResult &Usages, RangeDescriptor &Descriptor) {
   Descriptor.ContainerString =
       std::string(getContainerString(Context, Loop, ContainerExpr));
+  Descriptor.NeedsReverseCall = (FixerKind == LFK_ReverseIterator);
 
-  if (FixerKind == LFK_Iterator)
+  if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator)
     getIteratorLoopQualifiers(Context, Nodes, Descriptor);
   else
     getArrayLoopQualifiers(Context, Nodes, ContainerExpr, Usages, Descriptor);
@@ -792,7 +853,7 @@ bool LoopConvertCheck::isConvertible(ASTContext *Context,
     return false;
 
   // FIXME: Try to put most of this logic inside a matcher.
-  if (FixerKind == LFK_Iterator) {
+  if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator) {
     QualType InitVarType = InitVar->getType();
     QualType CanonicalInitVarType = InitVarType.getCanonicalType();
 
@@ -831,6 +892,8 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
     FixerKind = LFK_Array;
   } else if ((Loop = Nodes.getNodeAs<ForStmt>(LoopNameIterator))) {
     FixerKind = LFK_Iterator;
+  } else if ((Loop = Nodes.getNodeAs<ForStmt>(LoopNameReverseIterator))) {
+    FixerKind = LFK_ReverseIterator;
   } else {
     Loop = Nodes.getNodeAs<ForStmt>(LoopNamePseudoArray);
     assert(Loop && "Bad Callback. No for statement");
@@ -858,10 +921,11 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
   // With array loops, the container is often discovered during the
   // ForLoopIndexUseVisitor traversal.
   const Expr *ContainerExpr = nullptr;
-  if (FixerKind == LFK_Iterator) {
-    ContainerExpr = findContainer(Context, LoopVar->getInit(),
-                                  EndVar ? EndVar->getInit() : EndCall,
-                                  &Descriptor.ContainerNeedsDereference);
+  if (FixerKind == LFK_Iterator || FixerKind == LFK_ReverseIterator) {
+    ContainerExpr = findContainer(
+        Context, LoopVar->getInit(), EndVar ? EndVar->getInit() : EndCall,
+        &Descriptor.ContainerNeedsDereference,
+        /*IsReverse=*/FixerKind == LFK_ReverseIterator);
   } else if (FixerKind == LFK_PseudoArray) {
     ContainerExpr = EndCall->getImplicitObjectArgument();
     Descriptor.ContainerNeedsDereference =
@@ -927,6 +991,23 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
                Finder.aliasFromForInit(), Loop, Descriptor);
 }
 
+llvm::StringRef LoopConvertCheck::getReverseFunction() const {
+  if (!ReverseFunction.empty())
+    return ReverseFunction;
+  if (UseReverseRanges)
+    return "std::ranges::reverse_view";
+  return "";
+}
+
+llvm::StringRef LoopConvertCheck::getReverseHeader() const {
+  if (!ReverseHeader.empty())
+    return ReverseHeader;
+  if (UseReverseRanges && ReverseFunction.empty()) {
+    return "<ranges>";
+  }
+  return "";
+}
+
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h

@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H
 
 #include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
 #include "LoopConvertUtils.h"
 
 namespace clang {
@@ -24,6 +25,8 @@ class LoopConvertCheck : public ClangTidyCheck {
   }
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   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;
 
 private:
@@ -34,6 +37,7 @@ class LoopConvertCheck : public ClangTidyCheck {
     bool DerefByValue;
     std::string ContainerString;
     QualType ElemType;
+    bool NeedsReverseCall;
   };
 
   void getAliasRange(SourceManager &SM, SourceRange &DeclRange);
@@ -67,10 +71,18 @@ class LoopConvertCheck : public ClangTidyCheck {
   bool isConvertible(ASTContext *Context, const ast_matchers::BoundNodes &Nodes,
                      const ForStmt *Loop, LoopFixerKind FixerKind);
 
+  StringRef getReverseFunction() const;
+  StringRef getReverseHeader() const;
+
   std::unique_ptr<TUTrackingInfo> TUInfo;
   const unsigned long long MaxCopySize;
   const Confidence::Level MinConfidence;
   const VariableNamer::NamingStyle NamingStyle;
+  utils::IncludeInserter Inserter;
+  bool UseReverseRanges;
+  const bool UseCxx20IfAvailable;
+  std::string ReverseFunction;
+  std::string ReverseHeader;
 };
 
 } // namespace modernize

clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h

@@ -26,7 +26,12 @@ namespace clang {
 namespace tidy {
 namespace modernize {
 
-enum LoopFixerKind { LFK_Array, LFK_Iterator, LFK_PseudoArray };
+enum LoopFixerKind {
+  LFK_Array,
+  LFK_Iterator,
+  LFK_ReverseIterator,
+  LFK_PseudoArray
+};
 
 /// A map used to walk the AST in reverse: maps child Stmt to parent Stmt.
 typedef llvm::DenseMap<const clang::Stmt *, const clang::Stmt *> StmtParentMap;

clang-tools-extra/docs/ReleaseNotes.rst

@@ -114,6 +114,11 @@ New checks
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`modernize-loop-convert
+  <clang-tidy/checks/modernize-loop-convert>` check.
+
+  Now able to transform iterator loops using ``rbegin`` and ``rend`` methods.
+
 - Improved :doc:`readability-identifier-naming
   <clang-tidy/checks/readability-identifier-naming>` check.
 

clang-tools-extra/docs/clang-tidy/checks/modernize-loop-convert.rst

@@ -118,6 +118,48 @@ After applying the check with minimum confidence level set to `reasonable` (defa
   for (auto & elem : v)
     cout << elem;
 
+Reverse Iterator Support
+------------------------
+
+The converter is also capable of transforming iterator loops which use 
+``rbegin`` and ``rend`` for looping backwards over a container. Out of the box 
+this will automatically happen in C++20 mode using the ``ranges`` library, 
+however the check can be configured to work without C++20 by specifying a 
+function to reverse a range and optionally the header file where that function
+lives.
+
+.. option:: UseCxx20ReverseRanges
+  
+   When set to true convert loops when in C++20 or later mode using 
+   ``std::ranges::reverse_view``.
+   Default value is ``true``.
+
+.. option:: MakeReverseRangeFunction
+
+   Specify the function used to reverse an iterator pair, the function should 
+   accept a class with ``rbegin`` and ``rend`` methods and return a 
+   class with ``begin`` and ``end`` methods methods that call the ``rbegin`` and
+   ``rend`` methods respectively. Common examples are ``ranges::reverse_view``
+   and ``llvm::reverse``.
+   Default value is an empty string.
+
+.. option:: MakeReverseRangeHeader
+
+   Specifies the header file where :option:`MakeReverseRangeFunction` is
+   declared. For the previous examples this option would be set to 
+   ``range/v3/view/reverse.hpp`` and ``llvm/ADT/STLExtras.h`` respectively.
+   If this is an empty string and :option:`MakeReverseRangeFunction` is set, 
+   the check will proceed on the assumption that the function is already 
+   available in the translation unit.
+   This can be wrapped in angle brackets to signify to add the include as a
+   system include.
+   Default value is an empty string.
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
 Limitations
 -----------