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

556–557 ↗

(On Diff #34155)

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

658–663 ↗

(On Diff #34155)

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 ↗

(On Diff #34155)

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

28–44 ↗

(On Diff #34155)

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

658–663 ↗

(On Diff #34155)

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

658–663 ↗

(On Diff #34155)

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

clang-tidy/modernize/LoopConvertCheck.h

28 ↗

(On Diff #34158)

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 ↗

(On Diff #34160)

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 ↗

(On Diff #34160)

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 ↗

(On Diff #34160)

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-tools-extra/trunk/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-tools-extra/trunk/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-tools-extra/trunk/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);

clang-tools-extra/trunk/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