[PATCH] D12675: Avoid using rvalue references with trivially copyable types.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:556-557
@@ -555,1 +555,4 @@
         DerefByValue = true;
+        // We can assume that we have at least one Usage, because
+        // usagesAreConst() returned false.
+        QualType Type = Usages[0].Expression->getType().getCanonicalType();
----------------
I'd add (non-const):
... we have at least one (non-const) Usage...

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:658-663
@@ -646,7 +657,8 @@
+      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;
     }
 
----------------
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?

================
Comment at: clang-tidy/modernize/LoopConvertCheck.cpp:667
@@ -654,1 +666,3 @@
+        Nodes.getNodeAs<QualType>(DerefByValueResultName);
+    DerefByValue = DerefByValueType != nullptr;
     if (!DerefByValue) {
----------------
Btw, upstream style is to just use implicit null checks; probably not something you wrote, but good to change while we're at it.

================
Comment at: clang-tidy/modernize/LoopConvertCheck.h:28-44
@@ -27,18 +27,19 @@
 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;
----------------
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)?


http://reviews.llvm.org/D12675