[clang-tidy] Match derived types in in modernize-loop-convert

[ccotter]

Summary

This patch allows clang-tidy to replace traditional for-loops where the
container type inherits its begin/end methods from a base class.

Test plan: Added unit test cases that confirm the tool replaces the new
pattern.

[ccotter]

@njames93 would you mind taking a look?

[ccotter]

Add release note

[carlosgalvezp]

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

259

Replace tabs with spaces

clang-tools-extra/docs/ReleaseNotes.rst

215

Copy-paste typo?

[ccotter]

• Match derived types in in modernize-loop-convert
• fix typo

[ccotter]

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

259

Ah, I think this was before I figured out arc diff and was copy/pasting diff files around :/

Thanks for catching this.

[ccotter]

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

259

just to confirm, how did you notice the tabs? I couldnt find them when I downloaded the phab raw diff for any of the versions in the history. I still see a double right arrow » in the phab diff - is that indicating a tab or something else?

As a sanity check, I ran ./clang/tools/clang-format/git-clang-format --binary build/bin/clang-format from the root of my project and the tool did not produce any changes with the latest version of the changes I submitted.

[carlosgalvezp]

LGTM, let's give @njames93 a few days in case he has some comments.

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

259

You are right, it must be a bug in Phabricator!

[carlosgalvezp]

Hi! I'm finding merge conflicts when rebasing the patch on latest trunk, would you mind uploading a rebased version? Thanks!

[ccotter]

rebase

[ccotter]

rebase?

[ccotter]

Rebased! Sorry for the multiple diffs on this phab - I'm still getting the hang of arc diff.

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

 //===--- LoopConvertCheck.cpp - clang-tidy---------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
StatementMatcher makePseudoArrayLoopMatcher() {
   // return type of begin/end is an iterator that dereferences to the same as
   // what operator[] or at() returns. Such a test isn't likely to fail except
   // for pathological cases.
   //
   // FIXME: Also, a record doesn't necessarily need begin() and end(). Free
   // functions called begin() and end() taking the container as an argument
   // are also allowed.
   TypeMatcher RecordWithBeginEnd = qualType(anyOf(
-      qualType(
-          isConstQualified(),
-          hasUnqualifiedDesugaredType(recordType(hasDeclaration(cxxRecordDecl(
+      qualType(isConstQualified(),
+               hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+                   cxxRecordDecl(isSameOrDerivedFrom(cxxRecordDecl(
                        hasMethod(cxxMethodDecl(hasName("begin"), isConst())),
                        hasMethod(cxxMethodDecl(hasName("end"),
-                                      isConst()))))   // hasDeclaration
-                                                 ))), // qualType
+                                               isConst())))))) // hasDeclaration
+                                                      ))),     // qualType
       qualType(unless(isConstQualified()),
                hasUnqualifiedDesugaredType(recordType(hasDeclaration(
-                   cxxRecordDecl(hasMethod(hasName("begin")),
-                                 hasMethod(hasName("end"))))))) // qualType
+                   cxxRecordDecl(isSameOrDerivedFrom(cxxRecordDecl(
+                       hasMethod(hasName("begin")),
+                       hasMethod(hasName("end"))))))))) // qualType
       ));
 
   StatementMatcher SizeCallMatcher = cxxMemberCallExpr(
       argumentCountIs(0), callee(cxxMethodDecl(hasAnyName("size", "length"))),
       on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
                hasType(RecordWithBeginEnd))));
 
   StatementMatcher EndInitMatcher =

clang-tools-extra/docs/ReleaseNotes.rst

- Change the default behavior of :doc:`readability-const-return-type
   warn about `const` value parameters of declarations inside macros.
 
 - Improved :doc:`misc-redundant-expression <clang-tidy/checks/misc/redundant-expression>`
   check.
 
   The check now skips concept definitions since redundant expressions still make sense
   inside them.
 
 - Support removing ``c_str`` calls from ``std::string_view`` constructor calls in
   :doc: `readability-redundant-string-cstr <clang-tidy/checks/readability/redundant-string-cstr>`
   check.
 
+- Improved :doc:`modernize-loop-convert <clang-tidy/checks/modernize/loop-convert>`
+  to check for container functions ``begin``/``end`` etc on base classes of the container
+  type, instead of only as direct members of the container type itself.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/loop-convert/structures.h

 #ifndef STRUCTURES_H
 #define STRUCTURES_H
 
 extern "C" {
 extern int printf(const char *restrict, ...);
 }
 
 struct Val {int X; void g(); };
public:
 
   // Intentionally evil.
   dependent<ElemType> operator*();
 
   void foo();
   void constFoo() const;
 };
 
+template<typename ElemType>
+class dependent_derived : public dependent<ElemType> {
+};
+
 template<typename First, typename Second>
 class doublyDependent{
  public:
   struct Value {
     First first;
     Second second;
   };
 

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

 // RUN: %check_clang_tidy %s modernize-loop-convert %t -- -- -I %S/Inputs/loop-convert
 
 #include "structures.h"
 
 namespace Array {
 
 const int N = 6;
 const int NMinusOne = N - 1;

 
 const int N = 6;
 dependent<int> V;
 dependent<int> *Pv;
 const dependent<NonTriviallyCopyable> Constv;
 const dependent<NonTriviallyCopyable> *Pconstv;
 
 transparent<dependent<int>> Cv;
+dependent_derived<int> VD;
 
 void f() {
   int Sum = 0;
   for (int I = 0, E = V.size(); I < E; ++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
void f() {
   for (int I = 0, E = V.size(); 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 = VD.size(); E != I; ++I) {
+    printf("Fibonacci number is %d\n", VD[I]);
+    Sum += VD[I] + 2;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (int I : VD)
+  // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", I);
+  // CHECK-FIXES-NEXT: Sum += I + 2;
 }
 
 // Ensure that 'const auto &' is used with containers of non-trivial types.
 void constness() {
   int Sum = 0;
   for (int I = 0, E = Constv.size(); I < E; ++I) {
     printf("Fibonacci number is %d\n", Constv[I].X);
     Sum += Constv[I].X + 2;