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

[njames93]

Summary

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.

[Eugene.Zelenko]

It'll be interesting to run improved check over LLVM code base.

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

997

I think IsEnabled should be initialized as member or in constructor body because of non-trivial logic.

1003

Something need to be changed in: not, Disabling - comma to dot or capitalization.

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

123

Please separate with empty line.

141

Please highlight ranges::reverse_view and llvm::reverse with double back-ticks.

[njames93]

Address reviewer comments.

[njames93]

• Change default c++20 include to ranges instead of algorithm

[njames93]

In D82089#2100802, @Eugene.Zelenko wrote:

It'll be interesting to run improved check over LLVM code base.

I haven't tried running it over llvm, but I have had success using it with clangd in my editor, every loop I've tried seems to work a charm.

[njames93]

• Set and store the IncludeStyle

[njames93]

Add documentation about the include style optione

[njames93]

Rebased inline with new enum handling added in D82188

[njames93]

Updated release notes for version bump.

[njames93]

• Rebased trunk
• Cleaned up test cases
• Added support for specifying to include as system include.

[alexfh]

Thanks for the patch! Looks generally good. A few comments inline.

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

73–74

These are used in two places close together. I'd just use literals like for the other option names.

928

/*IsReverse=*/

1010

I'd just use string literals directly.

1018–1022

It looks like this should be a feature of the IncludeInserter. Not necessarily in this patch though. The createIncludeInsertion in other checks could be made a bit more self-descriptive too, e.g. this one in clang-tidy/modernize/MakeSmartPtrCheck.cpp:

Diag << Inserter.createIncludeInsertion(
    FD, MakeSmartPtrFunctionHeader,
    /*IsAngled=*/MakeSmartPtrFunctionHeader == StdMemoryHeader);

could have angle brackets in MakeSmartPtrFunctionHeader instead of making a special case for memory.

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

42–63

This abstraction doesn't seem to give much benefit over placing all these fields into the LoopConvertCheck class.

58

Use = false; for initialization. It's more common in LLVM.

[alexfh]

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

1018–1022

I've posted a patch for this for review: https://reviews.llvm.org/D85666.

[njames93]

Address reviewer comments

[njames93]

IsReverse=

[alexfh]

Looks good!

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

 //===--- LoopConvertCheck.h - clang-tidy-------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H
 
 #include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
 #include "LoopConvertUtils.h"
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 class LoopConvertCheck : public ClangTidyCheck {
 public:
   LoopConvertCheck(StringRef Name, ClangTidyContext *Context);
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus;
   }
   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:
   struct RangeDescriptor {
     RangeDescriptor();
     bool ContainerNeedsDereference;
     bool DerefByConstRef;
     bool DerefByValue;
     std::string ContainerString;
     QualType ElemType;
+    bool NeedsReverseCall;
   };
 
   void getAliasRange(SourceManager &SM, SourceRange &DeclRange);
 
   void doConversion(ASTContext *Context, const VarDecl *IndexVar,
                     const ValueDecl *MaybeContainer, const UsageResult &Usages,
                     const DeclStmt *AliasDecl, bool AliasUseRequired,
                     bool AliasFromForInit, const ForStmt *Loop,
                     RangeDescriptor Descriptor);
 
   StringRef getContainerString(ASTContext *Context, const ForStmt *Loop,
                                const Expr *ContainerExpr);
 
   void getArrayLoopQualifiers(ASTContext *Context,
                               const ast_matchers::BoundNodes &Nodes,
                               const Expr *ContainerExpr,
                               const UsageResult &Usages,
                               RangeDescriptor &Descriptor);
 
   void getIteratorLoopQualifiers(ASTContext *Context,
                                  const ast_matchers::BoundNodes &Nodes,
                                  RangeDescriptor &Descriptor);
 
   void determineRangeDescriptor(ASTContext *Context,
                                 const ast_matchers::BoundNodes &Nodes,
                                 const ForStmt *Loop, LoopFixerKind FixerKind,
                                 const Expr *ContainerExpr,
                                 const UsageResult &Usages,
                                 RangeDescriptor &Descriptor);
 
   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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H

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

 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #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>
 
 using namespace clang::ast_matchers;
 using namespace llvm;
 
 namespace clang {
getEnumMapping() {
     return makeArrayRef(Mapping);
   }
 };
 
 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";
 static const char IncrementVarName[] = "incrementVar";
 static const char InitVarName[] = "initVar";
 static const char BeginCallName[] = "beginCall";
 static const char EndCallName[] = "endCall";
 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(); }
 
 static const StatementMatcher IntegerComparisonMatcher() {
   return expr(ignoringParenImpCasts(
       declRefExpr(to(varDecl(hasType(isInteger())).bind(ConditionVarName)))));
 }
 
 static const DeclarationMatcher InitToZeroMatcher() {

 ///     ConditionEndVarName: 'g' (as a VarDecl)
 ///   In the second example only:
 ///     EndCallName: 'container.end()' (as a CXXMemberCallExpr)
 ///
 /// Client code will need to make sure that:
 ///   - 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 =
       varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher),
                                    materializeTemporaryExpr(
                                        ignoringParenImpCasts(BeginCallMatcher)),
                                    hasDescendant(BeginCallMatcher))))
           .bind(InitVarName);
 
   DeclarationMatcher EndDeclMatcher =
       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(
                      declRefExpr(to(varDecl().bind(ConditionEndVarName)))),
                  ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)),
                  materializeTemporaryExpr(ignoringParenImpCasts(
                      expr(EndCallMatcher).bind(EndCallName)))));
 
return forStmt(
                                hasUnaryOperand(declRefExpr(
                                    to(varDecl(hasType(pointsTo(AnyType())))
                                           .bind(IncrementVarName))))),
                  cxxOperatorCallExpr(
                      hasOverloadedOperatorName("++"),
                      hasArgument(
                          0, declRefExpr(to(varDecl(TestDerefReturnsByValue)
                                                .bind(IncrementVarName))))))))
-      .bind(LoopNameIterator);
+      .bind(IsReverse ? LoopNameReverseIterator : LoopNameIterator);
 }
 
 /// The matcher used for array-like containers (pseudoarrays).
 ///
 /// This matcher is more flexible than array-based loops. It will match
 /// loops of the following textual forms (regardless of whether the
 /// iterator type is actually a pointer type or a class type):
 ///

 }
 
 /// Determine whether Init appears to be an initializing an iterator.
 ///
 /// If it is, returns the object whose begin() or end() method is called, and
 /// 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));
   if (!TheCall || TheCall->getNumArgs() != 0)
     return nullptr;
 
   const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee());
   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();
   if (!SourceExpr)
     return nullptr;
 
   *IsArrow = Member->isArrow();
   return SourceExpr;
 }
 
 /// Determines the container whose begin() and end() functions are called
 /// for an iterator-based loop.
 ///
 /// BeginExpr must be a member call to a function named "begin()", and EndExpr
 /// 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 ||
       !areSameExpr(Context, EndContainerExpr, BeginContainerExpr))
     return nullptr;
 
   *ContainerNeedsDereference = BeginIsArrow;
   return BeginContainerExpr;
if (const auto *VDec = getReferencedVariable(ContainerExpr)) {
     CType = CType.getNonReferenceType();
     return CType.isConstQualified();
   }
   return false;
 }
 
 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:
 /// \code
 ///   unsigned &ThisIsADeclarationThatCanSpanSeveralLinesOfCode =
 ///       InitializationValues[I];
 ///   next_instruction;
 /// \endcode
if (!UseCopy) {
     } else if (Descriptor.DerefByValue) {
       if (!IsCheapToCopy)
         Type = Context->getRValueReferenceType(Type);
     } else {
       Type = Context->getLValueReferenceType(Type);
     }
   }
 
-  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));
 }
 
 /// Returns a string which refers to the container iterated over.
 StringRef LoopConvertCheck::getContainerString(ASTContext *Context,
                                                const ForStmt *Loop,
                                                const Expr *ContainerExpr) {

 
 /// Determines the parameters needed to build the range replacement.
 void LoopConvertCheck::determineRangeDescriptor(
     ASTContext *Context, const BoundNodes &Nodes, const ForStmt *Loop,
     LoopFixerKind FixerKind, const Expr *ContainerExpr,
     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);
 }
 
 /// Check some of the conditions that must be met for the loop to be
 /// convertible.
 bool LoopConvertCheck::isConvertible(ASTContext *Context,
bool LoopConvertCheck::isConvertible(ASTContext *Context,
   if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar))
     return false;
   const auto *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
   const auto *ConditionEndVar = Nodes.getNodeAs<VarDecl>(ConditionEndVarName);
   if (EndVar && !areSameVariable(EndVar, ConditionEndVar))
     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();
 
     const auto *BeginCall = Nodes.getNodeAs<CXXMemberCallExpr>(BeginCallName);
     assert(BeginCall && "Bad Callback. No begin call expression");
     QualType CanonicalBeginType =
         BeginCall->getMethodDecl()->getReturnType().getCanonicalType();
     if (CanonicalBeginType->isPointerType() &&
void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
   const ForStmt *Loop;
   LoopFixerKind FixerKind;
   RangeDescriptor Descriptor;
 
   if ((Loop = Nodes.getNodeAs<ForStmt>(LoopNameArray))) {
     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");
     FixerKind = LFK_PseudoArray;
   }
 
   if (!isConvertible(Context, Nodes, Loop, FixerKind))
     return;
void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
   const auto *BoundExpr = Nodes.getNodeAs<Expr>(ConditionBoundName);
 
   // Find container expression of iterators and pseudoarrays, and determine if
   // this expression needs to be dereferenced to obtain the container.
   // 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 =
         dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
   }
 
   // We must know the container or an array length bound.
   if (!ContainerExpr && !BoundExpr)
if (DependencyFinder.dependsOnInsideVariable(ContainerExpr) ||
       ConfidenceLevel.getLevel() < MinConfidence)
     return;
 
   doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr), Usages,
                Finder.getAliasDecl(), Finder.aliasUseRequired(),
                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/LoopConvertUtils.h

 #include <memory>
 #include <string>
 #include <utility>
 
 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;
 
 /// A map used to walk the AST in reverse:
 ///  maps VarDecl to the to parent DeclStmt.
 typedef llvm::DenseMap<const clang::VarDecl *, const clang::DeclStmt *>
     DeclParentMap;

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-redundant-branch-condition
   <clang-tidy/checks/bugprone-redundant-branch-condition>` check.
 
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
 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.
 
   Added an option `GetConfigPerFile` to support including files which use
   different naming styles.
 
 Improvements to include-fixer
 -----------------------------

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

.. code-block:: c++
   // reasonable conversion
   for (auto & elem : v)
     cout << elem;
 
   // reasonable conversion
   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
 -----------
 
 There are certain situations where the tool may erroneously perform
 transformations that remove information and change semantics. Users of the tool
 should be aware of the behaviour and limitations of the check outlined by
 the cases below.
 

clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-reverse.cpp

+// RUN: %check_clang_tidy -std=c++20 -check-suffixes=,RANGES %s modernize-loop-convert %t
+
+// RUN: %check_clang_tidy -check-suffixes=,CUSTOM,CUSTOM-NO-SYS %s modernize-loop-convert %t -- \
+// RUN:   -config="{CheckOptions: [ \
+// RUN:   {key: modernize-loop-convert.MakeReverseRangeFunction, value: 'llvm::reverse'}, \
+// RUN:   {key: modernize-loop-convert.MakeReverseRangeHeader, value: 'llvm/ADT/STLExtras.h'}]}"
+
+// RUN: %check_clang_tidy -check-suffixes=,CUSTOM,CUSTOM-SYS %s modernize-loop-convert %t -- \
+// RUN:   -config="{CheckOptions: [ \
+// RUN:   {key: modernize-loop-convert.MakeReverseRangeFunction, value: 'llvm::reverse'}, \
+// RUN:   {key: modernize-loop-convert.MakeReverseRangeHeader, value: '<llvm/ADT/STLExtras.h>'}]}"
+
+// RUN: %check_clang_tidy -check-suffixes=,CUSTOM,CUSTOM-NO-HEADER %s modernize-loop-convert %t -- \
+// RUN:   -config="{CheckOptions: [ \
+// RUN:   {key: modernize-loop-convert.MakeReverseRangeFunction, value: 'llvm::reverse'}]}"
+
+// Ensure the check doesn't transform reverse loops when not in c++20 mode or
+// when UseCxx20ReverseRanges has been disabled
+// RUN: clang-tidy %s -checks=-*,modernize-loop-convert -- -std=c++17 | count 0
+
+// RUN: clang-tidy %s -checks=-*,modernize-loop-convert -config="{CheckOptions: \
+// RUN:     [{key: modernize-loop-convert.UseCxx20ReverseRanges, value: 'false'}] \
+// RUN:     }" -- -std=c++20 | count 0
+
+// Ensure we get a warning if we supply the header argument without the
+// function argument.
+// RUN: clang-tidy %s -checks=-*,modernize-loop-convert -config="{CheckOptions: [ \
+// RUN:   {key: modernize-loop-convert.MakeReverseRangeHeader, value: 'llvm/ADT/STLExtras.h'}]}" \
+// RUN: -- -std=c++17 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=CHECK-HEADER-NO-FUNC \
+// RUN:       -implicit-check-not="{{warning|error}}:"
+
+// CHECK-HEADER-NO-FUNC: warning: modernize-loop-convert: 'MakeReverseRangeHeader' is set but 'MakeReverseRangeFunction' is not, disabling reverse loop transformation
+
+// Make sure appropiate headers are included
+// CHECK-FIXES-RANGES: #include <ranges>
+// CHECK-FIXES-CUSTOM-NO-SYS: #include "llvm/ADT/STLExtras.h"
+// CHECK-FIXES-CUSTOM-SYS: #include <llvm/ADT/STLExtras.h>
+
+// Make sure no header is included in this example
+// CHECK-FIXES-CUSTOM-NO-HEADER-NOT: #include
+
+template <typename T>
+struct Reversable {
+  using iterator = T *;
+  using const_iterator = const T *;
+
+  iterator begin();
+  iterator end();
+  iterator rbegin();
+  iterator rend();
+
+  const_iterator begin() const;
+  const_iterator end() const;
+  const_iterator rbegin() const;
+  const_iterator rend() const;
+
+  const_iterator cbegin() const;
+  const_iterator cend() const;
+  const_iterator crbegin() const;
+  const_iterator crend() const;
+};
+
+template <typename T>
+void observe(const T &);
+template <typename T>
+void mutate(T &);
+
+void constContainer(const Reversable<int> &Numbers) {
+  for (auto I = Numbers.rbegin(), E = Numbers.rend(); I != E; ++I) {
+    observe(*I);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES-RANGES: for (int Number : std::ranges::reverse_view(Numbers)) {
+  // CHECK-FIXES-CUSTOM: for (int Number : llvm::reverse(Numbers)) {
+  //   CHECK-FIXES-NEXT:   observe(Number);
+  //   CHECK-FIXES-NEXT: }
+
+  for (auto I = Numbers.crbegin(), E = Numbers.crend(); I != E; ++I) {
+    observe(*I);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES-RANGES: for (int Number : std::ranges::reverse_view(Numbers)) {
+  // CHECK-FIXES-CUSTOM: for (int Number : llvm::reverse(Numbers)) {
+  //   CHECK-FIXES-NEXT:   observe(Number);
+  //   CHECK-FIXES-NEXT: }
+
+  // Ensure these bad loops aren't transformed.
+  for (auto I = Numbers.rbegin(), E = Numbers.end(); I != E; ++I) {
+    observe(*I);
+  }
+  for (auto I = Numbers.begin(), E = Numbers.rend(); I != E; ++I) {
+    observe(*I);
+  }
+}
+
+void nonConstContainer(Reversable<int> &Numbers) {
+  for (auto I = Numbers.rbegin(), E = Numbers.rend(); I != E; ++I) {
+    mutate(*I);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES-RANGES: for (int & Number : std::ranges::reverse_view(Numbers)) {
+  // CHECK-FIXES-CUSTOM: for (int & Number : llvm::reverse(Numbers)) {
+  //   CHECK-FIXES-NEXT:   mutate(Number);
+  //   CHECK-FIXES-NEXT: }
+
+  for (auto I = Numbers.crbegin(), E = Numbers.crend(); I != E; ++I) {
+    observe(*I);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES-RANGES: for (int Number : std::ranges::reverse_view(Numbers)) {
+  // CHECK-FIXES-CUSTOM: for (int Number : llvm::reverse(Numbers)) {
+  //   CHECK-FIXES-NEXT:   observe(Number);
+  //   CHECK-FIXES-NEXT: }
+}