[clang-tidy] modernize-loop-convert reverse iteration support

Enables support for transforming loops of the form
```
for (auto I = Cont.rbegin(), E = Cont.rend(); I != E;++I)
```

This is done automatically in C++20 mode using `std::ranges::reverse_view` but there are options to specify a different function to reverse iterator over a container.
This is the first step, down the line I'd like to possibly extend this support for array based loops
```
for (unsigned I = Arr.size() - 1;I >=0;--I) Arr[I]...
```

Currently if you pass a reversing function with no header in the options it will just assume that the function exists, however as we have the ASTContext it may be as wise to check before applying, or at least lower the confidence level if we can't find it.

Reviewed By: alexfh

Differential Revision: https://reviews.llvm.org/D82089

Author: njames93

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
 -----------