ccotter updated this revision to Diff 546996.
ccotter added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140760/new/

https://reviews.llvm.org/D140760

Files:
  clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
  clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-negative.cpp
@@ -5,6 +5,22 @@
 // CHECK-FIXES-NOT: for ({{.*[^:]:[^:].*}})
 // CHECK-MESSAGES-NOT: modernize-loop-convert
 
+namespace somenamespace {
+  template <class T> auto begin(T& t) -> decltype(t.begin());
+  template <class T> auto begin(const T& t) -> decltype(t.begin());
+  template <class T> auto end(T& t) -> decltype(t.end());
+  template <class T> auto end(const T& t) -> decltype(t.end());
+  template <class T> auto size(const T& t) -> decltype(t.size());
+} // namespace somenamespace
+
+struct SomeClass {
+  template <class T> static auto begin(T& t) -> decltype(t.begin());
+  template <class T> static auto begin(const T& t) -> decltype(t.begin());
+  template <class T> static auto end(T& t) -> decltype(t.end());
+  template <class T> static auto end(const T& t) -> decltype(t.end());
+  template <class T> static auto size(const T& t) -> decltype(t.size());
+};
+
 namespace Negative {
 
 const int N = 6;
@@ -92,7 +108,7 @@
   }
 }
 
-}
+} // namespace Negative
 
 namespace NegativeIterator {
 
@@ -103,6 +119,10 @@
 struct BadBeginEnd : T {
   iterator notBegin();
   iterator notEnd();
+  iterator begin(int);
+  iterator end(int);
+  iterator begin();
+  iterator end();
 };
 
 void notBeginOrEnd() {
@@ -112,6 +132,9 @@
 
   for (T::iterator I = Bad.begin(), E = Bad.notEnd();  I != E; ++I)
     int K = *I;
+
+  for (T::iterator I = Bad.begin(0), E = Bad.end(0);  I != E; ++I)
+    int K = *I;
 }
 
 void badLoopShapes() {
@@ -202,6 +225,8 @@
   T Other;
   for (T::iterator I = Tt.begin(), E = Other.end();  I != E; ++I)
     int K = *I;
+  for (T::iterator I = begin(Tt), E = end(Other);  I != E; ++I)
+    int K = *I;
 
   for (T::iterator I = Other.begin(), E = Tt.end();  I != E; ++I)
     int K = *I;
@@ -214,6 +239,24 @@
     MutableVal K = *I;
 }
 
+void mixedMemberAndADL() {
+  for (T::iterator I = Tt.begin(), E = end(Tt);  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = begin(Tt), E = Tt.end();  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = std::begin(Tt), E = Tt.end();  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = std::begin(Tt), E = end(Tt);  I != E; ++I)
+    int K = *I;
+}
+
+void nonADLOrStdCalls() {
+  for (T::iterator I = SomeClass::begin(Tt), E = SomeClass::end(Tt);  I != E; ++I)
+    int K = *I;
+  for (T::iterator I = somenamespace::begin(Tt), E = somenamespace::end(Tt);  I != E; ++I)
+    int K = *I;
+}
+
 void wrongIterators() {
   T::iterator Other;
   for (T::iterator I = Tt.begin(), E = Tt.end(); I != Other; ++I)
@@ -379,6 +422,13 @@
     Sum += V[I];
 }
 
+void nonADLOrStdCalls() {
+  for (int I = 0, E = somenamespace::size(V); E != I; ++I)
+    printf("Fibonacci number is %d\n", V[I]);
+  for (int I = 0, E = SomeClass::size(V); E != I; ++I)
+    printf("Fibonacci number is %d\n", V[I]);
+}
+
 // Checks to see that non-const member functions are not called on the container
 // object.
 // These could be conceivably allowed with a lower required confidence level.
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp
@@ -445,6 +445,41 @@
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & I : Dpp)
   // CHECK-FIXES-NEXT: printf("%d\n", I->X);
+
+  for (S::iterator It = begin(Ss), E = end(Ss); It != E; ++It) {
+    printf("s0 has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : Ss)
+  // CHECK-FIXES-NEXT: printf("s0 has value %d\n", It.X);
+
+  for (S::iterator It = std::begin(Ss), E = std::end(Ss); It != E; ++It) {
+    printf("s1 has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : Ss)
+  // CHECK-FIXES-NEXT: printf("s1 has value %d\n", It.X);
+
+  for (S::iterator It = begin(*Ps), E = end(*Ps); It != E; ++It) {
+    printf("s2 has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : *Ps)
+  // CHECK-FIXES-NEXT: printf("s2 has value %d\n", It.X);
+
+  for (S::iterator It = begin(*Ps); It != end(*Ps); ++It) {
+    printf("s3 has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & It : *Ps)
+  // CHECK-FIXES-NEXT: printf("s3 has value %d\n", It.X);
+
+  for (S::const_iterator It = cbegin(Ss), E = cend(Ss); It != E; ++It) {
+    printf("s4 has value %d\n", (*It).X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto It : Ss)
+  // CHECK-FIXES-NEXT: printf("s4 has value %d\n", It.X);
 }
 
 // Tests to verify the proper use of auto where the init variable type and the
@@ -663,6 +698,43 @@
   // CHECK-FIXES: for (int I : VD)
   // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", I);
   // CHECK-FIXES-NEXT: Sum += I + 2;
+
+  for (int I = 0, E = size(V); E != I; ++I) {
+    printf("Fibonacci number is %d\n", V[I]);
+    Sum += V[I] + 2;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : V)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", I);
+  // CHECK-FIXES-NEXT: Sum += I + 2;
+
+  for (int I = 0, E = size(V); E != I; ++I) {
+    V[I] = 0;
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & I : V)
+  // CHECK-FIXES-NEXT: I = 0;
+
+  for (int I = 0, E = std::size(V); E != I; ++I) {
+    V[I] = 0;
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int & I : V)
+  // CHECK-FIXES-NEXT: I = 0;
+
+  // Although 'length' might be a valid free function, only size() is standardized
+  for (int I = 0, E = length(V); E != I; ++I) {
+    printf("Fibonacci number is %d\n", V[I]);
+    Sum += V[I] + 2;
+  }
+
+  dependent<Val> Vals;
+  for (int I = 0, E = size(Vals); E != I; ++I) {
+    Sum += Vals[I].X;
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & Val : Vals)
+  // CHECK-FIXES-NEXT: Sum += Val.X;
 }
 
 // Ensure that 'const auto &' is used with containers of non-trivial types.
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h
@@ -1,6 +1,14 @@
 #ifndef STRUCTURES_H
 #define STRUCTURES_H
 
+namespace std {
+template <class T> constexpr auto begin(T& t) -> decltype(t.begin());
+template <class T> constexpr auto begin(const T& t) -> decltype(t.begin());
+template <class T> constexpr auto end(T& t) -> decltype(t.end());
+template <class T> constexpr auto end(const T& t) -> decltype(t.end());
+template <class T> constexpr auto size(const T& t) -> decltype(t.size());
+} // namespace std
+
 extern "C" {
 extern int printf(const char *restrict, ...);
 }
@@ -28,6 +36,8 @@
   char Array[16];
 };
 
+namespace ADT {
+
 struct S {
   typedef MutableVal *iterator;
   typedef const MutableVal *const_iterator;
@@ -39,6 +49,14 @@
   iterator end();
 };
 
+S::const_iterator begin(const S&);
+S::const_iterator end(const S&);
+S::const_iterator cbegin(const S&);
+S::const_iterator cend(const S&);
+S::iterator begin(S&);
+S::iterator end(S&);
+unsigned size(const S&);
+
 struct T {
   typedef int value_type;
   struct iterator {
@@ -52,6 +70,13 @@
   iterator begin();
   iterator end();
 };
+T::iterator begin(T&);
+T::iterator end(T&);
+
+} // namespace ADT
+
+using ADT::S;
+using ADT::T;
 
 struct Q {
   typedef int value_type;
@@ -90,6 +115,8 @@
   S getS();
 };
 
+namespace ADT {
+
 template<typename ElemType>
 class dependent {
  public:
@@ -126,10 +153,20 @@
   void constFoo() const;
 };
 
+template<typename ElemType>
+unsigned size(const dependent<ElemType>&);
+template<typename ElemType>
+unsigned length(const dependent<ElemType>&);
+
 template<typename ElemType>
 class dependent_derived : public dependent<ElemType> {
 };
 
+} // namespace ADT
+
+using ADT::dependent;
+using ADT::dependent_derived;
+
 template<typename First, typename Second>
 class doublyDependent{
  public:
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/loop-convert.rst
@@ -91,10 +91,22 @@
   for (vector<int>::iterator it = v.begin(); it != v.end(); ++it)
     cout << *it;
 
+  // reasonable conversion
+  for (vector<int>::iterator it = begin(v); it != end(v); ++it)
+    cout << *it;
+
+  // reasonable conversion
+  for (vector<int>::iterator it = std::begin(v); it != std::end(v); ++it)
+    cout << *it;
+
   // reasonable conversion
   for (int i = 0; i < v.size(); ++i)
     cout << v[i];
 
+  // reasonable conversion
+  for (int i = 0; i < size(v); ++i)
+    cout << v[i];
+
 After applying the check with minimum confidence level set to `reasonable` (default):
 
 .. code-block:: c++
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -134,6 +134,10 @@
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Improved :doc:`modernize-loop-convert
+  <clang-tidy/checks/modernize/loop-convert>` to support for-loops with
+  iterators initialized by free functions like `begin`, `end`, or `size`.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -17,11 +17,13 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cstring>
 #include <optional>
+#include <tuple>
 #include <utility>
 
 using namespace clang::ast_matchers;
@@ -66,6 +68,12 @@
 static const char EndVarName[] = "endVar";
 static const char DerefByValueResultName[] = "derefByValueResult";
 static const char DerefByRefResultName[] = "derefByRefResult";
+static const llvm::StringSet<> ADLNames{"begin",   "cbegin", "rbegin",
+                                        "crbegin", "end",    "cend",
+                                        "rend",    "crend",  "size"};
+static const llvm::StringSet<> StdNames{
+    "std::begin", "std::cbegin", "std::rbegin", "std::crbegin", "std::end",
+    "std::cend",  "std::rend",   "std::crend",  "std::size"};
 
 static const StatementMatcher integerComparisonMatcher() {
   return expr(ignoringParenImpCasts(
@@ -129,6 +137,10 @@
 ///        e = createIterator(); it != e; ++it) { ... }
 ///   for (containerType::iterator it = container.begin();
 ///        it != anotherContainer.end(); ++it) { ... }
+///   for (containerType::iterator it = begin(container),
+///        e = end(container); it != e; ++it) { ... }
+///   for (containerType::iterator it = std::begin(container),
+///        e = std::end(container); it != e; ++it) { ... }
 /// \endcode
 /// The following string identifiers are bound to the parts of the AST:
 ///   InitVarName: 'it' (as a VarDecl)
@@ -137,6 +149,8 @@
 ///     EndVarName: 'e' (as a VarDecl)
 ///   In the second example only:
 ///     EndCallName: 'container.end()' (as a CXXMemberCallExpr)
+///   In the third/fourth examples:
+///     'end(container)' or 'std::end(container)' (as a CallExpr)
 ///
 /// Client code will need to make sure that:
 ///   - The two containers on which 'begin' and 'end' are called are the same.
@@ -144,13 +158,22 @@
 
   auto BeginNameMatcher = IsReverse ? hasAnyName("rbegin", "crbegin")
                                     : hasAnyName("begin", "cbegin");
+  auto BeginNameMatcherStd = IsReverse
+                                 ? hasAnyName("::std::rbegin", "::std::crbegin")
+                                 : hasAnyName("::std::begin", "::std::cbegin");
 
   auto EndNameMatcher =
       IsReverse ? hasAnyName("rend", "crend") : hasAnyName("end", "cend");
+  auto EndNameMatcherStd = IsReverse ? hasAnyName("::std::rend", "::std::crend")
+                                     : hasAnyName("::std::end", "::std::cend");
 
   StatementMatcher BeginCallMatcher =
-      cxxMemberCallExpr(argumentCountIs(0),
-                        callee(cxxMethodDecl(BeginNameMatcher)))
+      expr(anyOf(cxxMemberCallExpr(argumentCountIs(0),
+                                   callee(cxxMethodDecl(BeginNameMatcher))),
+                 callExpr(argumentCountIs(1),
+                          callee(functionDecl(BeginNameMatcher)), usesADL()),
+                 callExpr(argumentCountIs(1),
+                          callee(functionDecl(BeginNameMatcherStd)))))
           .bind(BeginCallName);
 
   DeclarationMatcher InitDeclMatcher =
@@ -163,8 +186,12 @@
   DeclarationMatcher EndDeclMatcher =
       varDecl(hasInitializer(anything())).bind(EndVarName);
 
-  StatementMatcher EndCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
+  StatementMatcher EndCallMatcher = expr(anyOf(
+      cxxMemberCallExpr(argumentCountIs(0),
+                        callee(cxxMethodDecl(EndNameMatcher))),
+      callExpr(argumentCountIs(1), callee(functionDecl(EndNameMatcher)),
+               usesADL()),
+      callExpr(argumentCountIs(1), callee(functionDecl(EndNameMatcherStd)))));
 
   StatementMatcher IteratorBoundMatcher =
       expr(anyOf(ignoringParenImpCasts(
@@ -223,6 +250,7 @@
 /// \code
 ///   for (int i = 0, j = container.size(); i < j; ++i) { ... }
 ///   for (int i = 0; i < container.size(); ++i) { ... }
+///   for (int i = 0; i < size(container); ++i) { ... }
 /// \endcode
 /// The following string identifiers are bound to the parts of the AST:
 ///   InitVarName: 'i' (as a VarDecl)
@@ -230,7 +258,8 @@
 ///   In the first example only:
 ///     EndVarName: 'j' (as a VarDecl)
 ///   In the second example only:
-///     EndCallName: 'container.size()' (as a CXXMemberCallExpr)
+///     EndCallName: 'container.size()' (as a CXXMemberCallExpr) or
+///     'size(contaner)' (as a CallExpr)
 ///
 /// Client code will need to make sure that:
 ///   - The containers on which 'size()' is called is the container indexed.
@@ -265,10 +294,15 @@
                        hasMethod(hasName("end"))))))))) // qualType
       ));
 
-  StatementMatcher SizeCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("size", "length"))),
-      on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
-               hasType(RecordWithBeginEnd))));
+  StatementMatcher SizeCallMatcher = expr(anyOf(
+      cxxMemberCallExpr(argumentCountIs(0),
+                        callee(cxxMethodDecl(hasAnyName("size", "length"))),
+                        on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
+                                 hasType(RecordWithBeginEnd)))),
+      callExpr(argumentCountIs(1), callee(functionDecl(hasName("size"))),
+               usesADL()),
+      callExpr(argumentCountIs(1),
+               callee(functionDecl(hasName("::std::size"))))));
 
   StatementMatcher EndInitMatcher =
       expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)),
@@ -296,36 +330,86 @@
       .bind(LoopNamePseudoArray);
 }
 
+enum IteratorCallKind {
+  ICK_Member,
+  ICK_ADL,
+  ICK_Std,
+};
+
+struct ContainerCall {
+  const Expr *Container;
+  StringRef Name;
+  bool IsArrow;
+  IteratorCallKind CallKind;
+};
+
+// Find the Expr likely initializing an iterator.
+//
+// Call is either a CXXMemberCallExpr ('c.begin()') or CallExpr of a free
+// function with the first argument as a container ('begin(c)'), or nullptr.
+// Returns at a 3-tuple with the container expr, function name (begin/end/etc),
+// and whether the call is made through an arrow (->) for CXXMemberCallExprs.
+// The returned Expr* is nullptr if any of the assumptions are not met.
+// static std::tuple<const Expr *, StringRef, bool, IteratorCallKind>
+static std::optional<ContainerCall> getContainerExpr(const Expr *Call) {
+  const Expr *Dug = digThroughConstructorsConversions(Call);
+
+  IteratorCallKind CallKind = ICK_Member;
+
+  if (const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(Dug)) {
+    CallKind = ICK_Member;
+    if (const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee()))
+      return ContainerCall{TheCall->getImplicitObjectArgument(),
+                           Member->getMemberDecl()->getName(),
+                           Member->isArrow(), CallKind};
+    else
+      return ContainerCall{TheCall->getArg(0),
+                           TheCall->getDirectCallee()->getName(), false,
+                           CallKind};
+  } else if (const auto *TheCall = dyn_cast_or_null<CallExpr>(Dug)) {
+    if (TheCall->getNumArgs() != 1)
+      return std::nullopt;
+
+    if (TheCall->usesADL()) {
+      if (!ADLNames.contains(TheCall->getDirectCallee()->getName()))
+        return std::nullopt;
+      CallKind = ICK_ADL;
+    } else {
+      if (!StdNames.contains(
+              TheCall->getDirectCallee()->getQualifiedNameAsString()))
+        return std::nullopt;
+      CallKind = ICK_Std;
+    }
+
+    return ContainerCall{TheCall->getArg(0),
+                         TheCall->getDirectCallee()->getName(), false,
+                         CallKind};
+  }
+  return std::nullopt;
+}
+
 /// 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 IsReverse) {
+static std::pair<const Expr *, IteratorCallKind>
+getContainerFromBeginEndCall(const Expr *Init, bool IsBegin, bool *IsArrow,
+                             bool IsReverse) {
   // FIXME: Maybe allow declaration/initialization outside of the for loop.
-  const auto *TheCall = dyn_cast_or_null<CXXMemberCallExpr>(
-      digThroughConstructorsConversions(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();
-  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;
+  std::optional<ContainerCall> Call = getContainerExpr(Init);
+  if (!Call)
+    return {};
+
+  *IsArrow = Call->IsArrow;
+  if (!Call->Name.consume_back(IsBegin ? "begin" : "end"))
+    return {};
+  if (IsReverse && !Call->Name.consume_back("r"))
+    return {};
+  if (!Call->Name.empty() && !Call->Name.equals("c"))
+    return {};
+  return std::make_pair(Call->Container, Call->CallKind);
 }
 
 /// Determines the container whose begin() and end() functions are called
@@ -341,13 +425,16 @@
   // valid.
   bool BeginIsArrow = false;
   bool EndIsArrow = false;
-  const Expr *BeginContainerExpr = getContainerFromBeginEndCall(
+  auto [BeginContainerExpr, BeginCallKind] = getContainerFromBeginEndCall(
       BeginExpr, /*IsBegin=*/true, &BeginIsArrow, IsReverse);
   if (!BeginContainerExpr)
     return nullptr;
 
-  const Expr *EndContainerExpr = getContainerFromBeginEndCall(
+  auto [EndContainerExpr, EndCallKind] = getContainerFromBeginEndCall(
       EndExpr, /*IsBegin=*/false, &EndIsArrow, IsReverse);
+  if (BeginCallKind != EndCallKind)
+    return nullptr;
+
   // 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 ||
@@ -832,10 +919,10 @@
     QualType InitVarType = InitVar->getType();
     QualType CanonicalInitVarType = InitVarType.getCanonicalType();
 
-    const auto *BeginCall = Nodes.getNodeAs<CXXMemberCallExpr>(BeginCallName);
+    const auto *BeginCall = Nodes.getNodeAs<CallExpr>(BeginCallName);
     assert(BeginCall && "Bad Callback. No begin call expression");
     QualType CanonicalBeginType =
-        BeginCall->getMethodDecl()->getReturnType().getCanonicalType();
+        BeginCall->getDirectCallee()->getReturnType().getCanonicalType();
     if (CanonicalBeginType->isPointerType() &&
         CanonicalInitVarType->isPointerType()) {
       // If the initializer and the variable are both pointers check if the
@@ -846,10 +933,12 @@
         return false;
     }
   } else if (FixerKind == LFK_PseudoArray) {
-    // This call is required to obtain the container.
-    const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
-    if (!EndCall || !isa<MemberExpr>(EndCall->getCallee()))
-      return false;
+    if (const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName)) {
+      // This call is required to obtain the container.
+      if (!isa<MemberExpr>(EndCall->getCallee()))
+        return false;
+    } else
+      return Nodes.getNodeAs<CallExpr>(EndCallName) != nullptr;
   }
   return true;
 }
@@ -888,7 +977,7 @@
 
   // If the end comparison isn't a variable, we can try to work with the
   // expression the loop variable is being tested against instead.
-  const auto *EndCall = Nodes.getNodeAs<CXXMemberCallExpr>(EndCallName);
+  const auto *EndCall = Nodes.getNodeAs<Expr>(EndCallName);
   const auto *BoundExpr = Nodes.getNodeAs<Expr>(ConditionBoundName);
 
   // Find container expression of iterators and pseudoarrays, and determine if
@@ -902,9 +991,11 @@
         &Descriptor.ContainerNeedsDereference,
         /*IsReverse=*/FixerKind == LFK_ReverseIterator);
   } else if (FixerKind == LFK_PseudoArray) {
-    ContainerExpr = EndCall->getImplicitObjectArgument();
-    Descriptor.ContainerNeedsDereference =
-        dyn_cast<MemberExpr>(EndCall->getCallee())->isArrow();
+    std::optional<ContainerCall> Call = getContainerExpr(EndCall);
+    if (Call) {
+      ContainerExpr = Call->Container;
+      Descriptor.ContainerNeedsDereference = Call->IsArrow;
+    }
   }
 
   // We must know the container or an array length bound.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to