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

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

658–663

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

28–44

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

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

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

658–663

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

556–565

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

658–663

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:
   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);
+                    bool DerefByValue, bool IsTriviallyCopyable,
+                    bool DerefByConstRef);
 
   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);
+                           bool IsTriviallyCopyable, bool DerefByConstRef,
+                           const ForStmt *TheLoop, LoopFixerKind FixerKind);
   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

 
 /// \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) {
+    bool IsTriviallyCopyable, bool DerefByConstRef) {
   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 (!IsTriviallyCopyable)
         AutoRefType = Context->getRValueReferenceType(AutoRefType);
     } else {
       if (DerefByConstRef)
         AutoRefType = Context->getConstType(AutoRefType);
       AutoRefType = Context->getLValueReferenceType(AutoRefType);
     }
   }
 
   StringRef MaybeDereference = ContainerNeedsDereference ? "*" : "";
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) {
+    bool ContainerNeedsDereference, bool DerefByValue, bool IsTriviallyCopyable,
+    bool DerefByConstRef, const ForStmt *TheLoop, LoopFixerKind FixerKind) {
   ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
                                 BoundExpr, ContainerNeedsDereference);
 
   if (ContainerExpr) {
     ComponentFinderASTVisitor ComponentFinder;
     ComponentFinder.findExprComponents(ContainerExpr->IgnoreParenImpCasts());
     Finder.addComponents(ComponentFinder.getComponents());
   }
if (FixerKind == LFK_Array) {
     // 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) {
       const UsageResult &Usages = Finder.getUsages();
       if (usagesAreConst(Usages)) {
-        // FIXME: check if the type is trivially copiable.
         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;
+        // We can assume that we have at least one Usage, because
+        // usagesAreConst() returned false.
+        QualType Type = Usages[0].Expression->getType().getCanonicalType();
+        if (Usages[0].IsArrow) {
+          if (!Type->isPointerType())
+            return;
+          Type = Type->getPointeeType();
+        }
+        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);
+               ContainerNeedsDereference, DerefByValue, IsTriviallyCopyable,
+               DerefByConstRef);
 }
 
 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 *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 IsTriviallyCopyable = false;
   bool DerefByConstRef = false;
   bool ContainerNeedsDereference = 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);
 
     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;
+      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;
+    const auto *DerefByValueType =
+        Nodes.getNodeAs<QualType>(DerefByValueResultName);
+    DerefByValue = DerefByValueType != nullptr;
     if (!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)
                               ->getAs<ReferenceType>()
if (!DerefByValue) {
         // otherwise we test for const qualification of the pointed-at type.
         if (!Context->hasSameType(InitPointeeType, BeginPointeeType))
           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();
+      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();
   }
 
   // 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);
+                      ContainerNeedsDereference, DerefByValue,
+                      IsTriviallyCopyable, DerefByConstRef, TheLoop, FixerKind);
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

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) {
 
 }
 
 } // namespace PseudoArray