Avoid using rvalue references with trivially copyable types.

[angelgarcia]

Summary

When the dereference operator returns a value that is trivially
copyable (like a pointer), copy it. After this change, modernize-loop-convert
check can be applied to the whole llvm source code without breaking any build
or test.

[alexfh]

Awesome! Looks good.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

555–556

I'd add (non-const):
... we have at least one (non-const) Usage...

659–664

Can you add a comment what FixerKind is in the else, so it's obvious why we don't want to do anything else in here?

667

Btw, upstream style is to just use implicit null checks; probably not something you wrote, but good to change while we're at it.

clang-tidy/modernize/LoopConvertCheck.h

35–49

I find it somewhat distressing that the variable order is completely different. Perhaps it's time to pull out a struct that encapsulates the specific attributes of the loop (the bools)?

[angelgarcia]

clang-tidy/modernize/LoopConvertCheck.cpp

659–664

What do you mean? We're still inside the "if (FixerKind == LFK_Iterator)".

[angelgarcia]

Add struct with the parameters of the replacement. Also fix a potential source of crashes from a previous patch.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

659–664

Ah, sorry, misread. So, why don't we want to check for trivial copy-ability for all types?

clang-tidy/modernize/LoopConvertCheck.h

28

Add a comment.

[angelgarcia]

Avoid relying in a single usage for determining if the type is trivially copyable.

[angelgarcia]

clang-tidy/modernize/LoopConvertCheck.cpp

659–664

Yes, but the arrays never return rvalues, and the iterator and pseudoarrays are "handled" in different parts in the code. I think I updated both of them, and I ensured that there was a test for each.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

555–564

a) please add a regression test for everything you find during development
b) I think the comment that we can assume that we have at least one non-const usage is not helpful any more, as we're not just accessing [0] any more.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

659–664

Ah, ok. I think that code is really hard to follow, and we'll want to refactor the flow (not in this patch though).

[angelgarcia]

This is the test case I was worried about.

[klimek]

LG

clang-tidy/modernize/LoopConvertCheck.h

 class LoopConvertCheck : public ClangTidyCheck {
 public:
   LoopConvertCheck(StringRef Name, ClangTidyContext *Context);
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
+  struct RangeDescriptor {
+    bool ContainerNeedsDereference;
+    bool DerefByValue;
+    bool IsTriviallyCopyable;
+    bool DerefByConstRef;
+  };
+
   void doConversion(ASTContext *Context, const VarDecl *IndexVar,
                     const VarDecl *MaybeContainer, StringRef ContainerString,
                     const UsageResult &Usages, const DeclStmt *AliasDecl,
                     bool AliasUseRequired, bool AliasFromForInit,
-                    const ForStmt *TheLoop, bool ContainerNeedsDereference,
-                    bool DerefByValue, bool DerefByConstRef);
+                    const ForStmt *TheLoop, RangeDescriptor Descriptor);
 
   StringRef checkRejections(ASTContext *Context, const Expr *ContainerExpr,
                             const ForStmt *TheLoop);
 
   void findAndVerifyUsages(ASTContext *Context, const VarDecl *LoopVar,
                            const VarDecl *EndVar, const Expr *ContainerExpr,
-                           const Expr *BoundExpr,
-                           bool ContainerNeedsDereference, bool DerefByValue,
-                           bool DerefByConstRef, const ForStmt *TheLoop,
-                           LoopFixerKind FixerKind);
+                           const Expr *BoundExpr, const ForStmt *TheLoop,
+                           LoopFixerKind FixerKind, RangeDescriptor Descriptor);
+
   std::unique_ptr<TUTrackingInfo> TUInfo;
   Confidence::Level MinConfidence;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H

clang-tidy/modernize/LoopConvertCheck.cpp

static bool usagesAreConst(const UsageResult &Usages) {
   // FIXME: Make this function more generic.
   return Usages.empty();
 }
 
 /// \brief Returns true if the elements of the container are never accessed
 /// by reference.
 static bool usagesReturnRValues(const UsageResult &Usages) {
   for (const auto &U : Usages) {
-    if (!U.Expression->isRValue())
+    if (U.Expression && !U.Expression->isRValue())
       return false;
   }
   return true;
 }
 
 LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
       MinConfidence(StringSwitch<Confidence::Level>(
                         Options.get("MinConfidence", "reasonable"))
                         .Case("safe", Confidence::CL_Safe)
                         .Case("risky", Confidence::CL_Risky)
                         .Default(Confidence::CL_Reasonable)) {}
 
 void LoopConvertCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   SmallVector<std::string, 3> Confs{"risky", "reasonable", "safe"};
   Options.store(Opts, "MinConfidence", Confs[static_cast<int>(MinConfidence)]);
 }
 
 /// \brief Computes the changes needed to convert a given for loop, and
 /// applies it.
 void LoopConvertCheck::doConversion(
     ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer,
     StringRef ContainerString, const UsageResult &Usages,
     const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit,
-    const ForStmt *TheLoop, bool ContainerNeedsDereference, bool DerefByValue,
-    bool DerefByConstRef) {
+    const ForStmt *TheLoop, RangeDescriptor Descriptor) {
   auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead");
 
   std::string VarName;
   bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
   bool AliasVarIsRef = false;
 
   if (VarNameFromAlias) {
     const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
void LoopConvertCheck::doConversion(
 
   // If the new variable name is from the aliased variable, then the reference
   // type for the new variable should only be used if the aliased variable was
   // declared as a reference.
   if (!VarNameFromAlias || AliasVarIsRef) {
     // If an iterator's operator*() returns a 'T&' we can bind that to 'auto&'.
     // If operator*() returns 'T' we can bind that to 'auto&&' which will deduce
     // to 'T&&&'.
-    if (DerefByValue) {
+    if (Descriptor.DerefByValue) {
+      if (!Descriptor.IsTriviallyCopyable)
         AutoRefType = Context->getRValueReferenceType(AutoRefType);
     } else {
-      if (DerefByConstRef)
+      if (Descriptor.DerefByConstRef)
         AutoRefType = Context->getConstType(AutoRefType);
       AutoRefType = Context->getLValueReferenceType(AutoRefType);
     }
   }
 
-  StringRef MaybeDereference = ContainerNeedsDereference ? "*" : "";
+  StringRef MaybeDereference = Descriptor.ContainerNeedsDereference ? "*" : "";
   std::string TypeString = AutoRefType.getAsString();
   std::string Range = ("(" + TypeString + " " + VarName + " : " +
                        MaybeDereference + ContainerString + ")")
                           .str();
   Diag << FixItHint::CreateReplacement(
       CharSourceRange::getTokenRange(ParenRange), Range);
   TUInfo->getGeneratedDecls().insert(make_pair(TheLoop, VarName));
 }
StringRef LoopConvertCheck::checkRejections(ASTContext *Context,
 
   return ContainerString;
 }
 
 /// \brief Given a loop header that would be convertible, discover all usages
 /// of the index variable and convert the loop if possible.
 void LoopConvertCheck::findAndVerifyUsages(
     ASTContext *Context, const VarDecl *LoopVar, const VarDecl *EndVar,
-    const Expr *ContainerExpr, const Expr *BoundExpr,
-    bool ContainerNeedsDereference, bool DerefByValue, bool DerefByConstRef,
-    const ForStmt *TheLoop, LoopFixerKind FixerKind) {
+    const Expr *ContainerExpr, const Expr *BoundExpr, const ForStmt *TheLoop,
+    LoopFixerKind FixerKind, RangeDescriptor Descriptor) {
   ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
-                                BoundExpr, ContainerNeedsDereference);
+                                BoundExpr,
+                                Descriptor.ContainerNeedsDereference);
 
   if (ContainerExpr) {
     ComponentFinderASTVisitor ComponentFinder;
     ComponentFinder.findExprComponents(ContainerExpr->IgnoreParenImpCasts());
     Finder.addComponents(ComponentFinder.getComponents());
   }
 
   if (!Finder.findAndVerifyUsages(TheLoop->getBody()))
     return;
 
   Confidence ConfidenceLevel(Finder.getConfidenceLevel());
   if (FixerKind == LFK_Array) {
     // The array being indexed by IndexVar was discovered during traversal.
     ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts();
     // Very few loops are over expressions that generate arrays rather than
     // array variables. Consider loops over arrays that aren't just represented
     // by a variable to be risky conversions.
     if (!getReferencedVariable(ContainerExpr) &&
         !isDirectMemberExpr(ContainerExpr))
       ConfidenceLevel.lowerTo(Confidence::CL_Risky);
   } else if (FixerKind == LFK_PseudoArray) {
-    if (!DerefByValue && !DerefByConstRef) {
+    if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) {
       const UsageResult &Usages = Finder.getUsages();
       if (usagesAreConst(Usages)) {
-        // FIXME: check if the type is trivially copiable.
-        DerefByConstRef = true;
+        Descriptor.DerefByConstRef = true;
       } else if (usagesReturnRValues(Usages)) {
         // If the index usages (dereference, subscript, at) return RValues,
         // then we should not use a non-const reference.
-        DerefByValue = true;
+        Descriptor.DerefByValue = true;
+        // Try to find the type of the elements on the container from the
+        // usages.
+        for (const Usage &U : Usages) {
+          if (!U.Expression || U.Expression->getType().isNull())
+            continue;
+          QualType Type = U.Expression->getType().getCanonicalType();
+          if (U.IsArrow) {
+            if (!Type->isPointerType())
+              continue;
+            Type = Type->getPointeeType();
+          }
+          Descriptor.IsTriviallyCopyable =
+              Type.isTriviallyCopyableType(*Context);
+        }
       }
     }
   }
 
   StringRef ContainerString = checkRejections(Context, ContainerExpr, TheLoop);
 
   if (ContainerString.empty() || ConfidenceLevel.getLevel() < MinConfidence)
     return;
 
   doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
                ContainerString, Finder.getUsages(), Finder.getAliasDecl(),
                Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop,
-               ContainerNeedsDereference, DerefByValue, DerefByConstRef);
+               Descriptor);
 }
 
 void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matchers for C++. Because this checker is used for
   // modernization, it is reasonable to run it on any C++ standard with the
   // assumption the user is trying to modernize their codebase.
   if (getLangOpts().CPlusPlus) {
     Finder->addMatcher(makeArrayLoopMatcher(), this);
void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName);
   const auto *BoundExpr = Nodes.getStmtAs<Expr>(ConditionBoundName);
   // If the loop calls end()/size() after each iteration, lower our confidence
   // level.
   if (FixerKind != LFK_Array && !EndVar)
     ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
 
   const Expr *ContainerExpr = nullptr;
-  bool DerefByValue = false;
-  bool DerefByConstRef = false;
-  bool ContainerNeedsDereference = false;
+  RangeDescriptor Descriptor{false, false, false, false};
   // FIXME: Try to put most of this logic inside a matcher. Currently, matchers
   // don't allow the ight-recursive checks in digThroughConstructors.
   if (FixerKind == LFK_Iterator) {
     ContainerExpr = findContainer(Context, LoopVar->getInit(),
                                   EndVar ? EndVar->getInit() : EndCall,
-                                  &ContainerNeedsDereference);
+                                  &Descriptor.ContainerNeedsDereference);
 
     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() &&
         CanonicalInitVarType->isPointerType()) {
       QualType BeginPointeeType = CanonicalBeginType->getPointeeType();
       QualType InitPointeeType = CanonicalInitVarType->getPointeeType();
       // If the initializer and the variable are both pointers check if the
       // un-qualified pointee types match otherwise we don't use auto.
       if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType))
         return;
+      Descriptor.IsTriviallyCopyable =
+          BeginPointeeType.isTriviallyCopyableType(*Context);
     } else {
       // Check for qualified types to avoid conversions from non-const to const
       // iterator types.
       if (!Context->hasSameType(CanonicalInitVarType, CanonicalBeginType))
         return;
     }
 
-    DerefByValue = Nodes.getNodeAs<QualType>(DerefByValueResultName) != nullptr;
-    if (!DerefByValue) {
+    const auto *DerefByValueType =
+        Nodes.getNodeAs<QualType>(DerefByValueResultName);
+    Descriptor.DerefByValue = DerefByValueType;
+    if (!Descriptor.DerefByValue) {
       if (const auto *DerefType =
               Nodes.getNodeAs<QualType>(DerefByRefResultName)) {
         // A node will only be bound with DerefByRefResultName if we're dealing
         // with a user-defined iterator type. Test the const qualification of
         // the reference type.
-        DerefByConstRef = (*DerefType)
+        Descriptor.DerefByConstRef = (*DerefType)
                                          ->getAs<ReferenceType>()
                                          ->getPointeeType()
                                          .isConstQualified();
       } else {
         // By nature of the matcher this case is triggered only for built-in
         // iterator types (i.e. pointers).
         assert(isa<PointerType>(CanonicalInitVarType) &&
                "Non-class iterator type is not a pointer type");
         QualType InitPointeeType = CanonicalInitVarType->getPointeeType();
         QualType BeginPointeeType = CanonicalBeginType->getPointeeType();
         // If the initializer and variable have both the same type just use auto
         // otherwise we test for const qualification of the pointed-at type.
         if (!Context->hasSameType(InitPointeeType, BeginPointeeType))
-          DerefByConstRef = InitPointeeType.isConstQualified();
+          Descriptor.DerefByConstRef = InitPointeeType.isConstQualified();
       }
     } else {
       // If the dereference operator returns by value then test for the
       // canonical const qualification of the init variable type.
-      DerefByConstRef = CanonicalInitVarType.isConstQualified();
+      Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified();
+      Descriptor.IsTriviallyCopyable =
+          DerefByValueType->isTriviallyCopyableType(*Context);
     }
   } else if (FixerKind == LFK_PseudoArray) {
     if (!EndCall)
       return;
     ContainerExpr = EndCall->getImplicitObjectArgument();
     const auto *Member = dyn_cast<MemberExpr>(EndCall->getCallee());
     if (!Member)
       return;
-    ContainerNeedsDereference = Member->isArrow();
+    Descriptor.ContainerNeedsDereference = Member->isArrow();
   }
 
   // We must know the container or an array length bound.
   if (!ContainerExpr && !BoundExpr)
     return;
 
   if (ConfidenceLevel.getLevel() < MinConfidence)
     return;
 
   findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr,
-                      ContainerNeedsDereference, DerefByValue, DerefByConstRef,
-                      TheLoop, FixerKind);
+                      TheLoop, FixerKind, Descriptor);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tidy/modernize/LoopConvertUtils.cpp

static bool isAliasDecl(ASTContext *Context, const Decl *TheDecl,
 
   const Expr *Init =
       digThroughConstructors(VDecl->getInit()->IgnoreParenImpCasts());
   if (!Init)
     return false;
 
   // Check that the declared type is the same as (or a reference to) the
   // container type.
+  QualType InitType = Init->getType();
   QualType DeclarationType = VDecl->getType();
-  if (DeclarationType->isReferenceType())
+  if (!DeclarationType.isNull() && DeclarationType->isReferenceType())
     DeclarationType = DeclarationType.getNonReferenceType();
-  QualType InitType = Init->getType();
-  if (!Context->hasSameUnqualifiedType(DeclarationType, InitType))
+
+  if (InitType.isNull() || DeclarationType.isNull() ||
+      !Context->hasSameUnqualifiedType(DeclarationType, InitType))
     return false;
 
   switch (Init->getStmtClass()) {
   case Stmt::ArraySubscriptExprClass: {
     const auto *E = cast<ArraySubscriptExpr>(Init);
     // We don't really care which array is used here. We check to make sure
     // it was the correct one later, since the AST will traverse it next.
     return isIndexInSubscriptExpr(E->getIdx(), IndexVar);

test/clang-tidy/modernize-loop-convert-basic.cpp

void f() {
   // PtrSet's iterator dereferences by value so auto & can't be used.
   {
     PtrSet<int *> int_ptrs;
     for (PtrSet<int *>::iterator I = int_ptrs.begin(),
                                  E = int_ptrs.end();
          I != E; ++I) {
     }
     // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead
-    // CHECK-FIXES: for (auto && int_ptr : int_ptrs) {
+    // CHECK-FIXES: for (auto int_ptr : int_ptrs) {
   }
 
   // This container uses an iterator where the derefence type is a typedef of
   // a reference type. Make sure non-const auto & is still used. A failure here
   // means canonical types aren't being tested.
   {
     TypedefDerefContainer<int> int_ptrs;
     for (TypedefDerefContainer<int>::iterator I = int_ptrs.begin(),
struct DerefByValue {
   DerefByValue();
   struct iter { unsigned operator*(); };
   unsigned size() const;
   iter begin();
   iter end();
   unsigned operator[](int);
 };
 
-void DerefByValueTest() {
+void derefByValueTest() {
   DerefByValue DBV;
   for (unsigned i = 0, e = DBV.size(); i < e; ++i) {
     printf("%d\n", DBV[i]);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
-  // CHECK-FIXES: for (auto && elem : DBV) {
+  // CHECK-FIXES: for (auto elem : DBV) {
+  // CHECK-FIXES-NEXT: printf("%d\n", elem);
 
+  for (unsigned i = 0, e = DBV.size(); i < e; ++i) {
+    auto f = [DBV, i]() {};
+    printf("%d\n", DBV[i]);
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto elem : DBV) {
+  // CHECK-FIXES-NEXT: auto f = [DBV, elem]() {};
+  // CHECK-FIXES-NEXT: printf("%d\n", elem);
 }
 
 } // namespace PseudoArray