Only copy small types in modernize-loop-convert.

[angelgarcia]

Summary

If the size of the type is above a certain bound, we'll take a const reference. This bound can be set as an option. For now, the default value is 16 bytes.

[klimek]

lg

clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h

void determineRangeDescriptor(ASTContext *Context,
                                 const Expr *ContainerExpr,
                                 const UsageResult &Usages,
                                 RangeDescriptor &Descriptor);
 
   bool isConvertible(ASTContext *Context, const ast_matchers::BoundNodes &Nodes,
                      const ForStmt *Loop, LoopFixerKind FixerKind);
 
   std::unique_ptr<TUTrackingInfo> TUInfo;
+  const unsigned long long MaxCopySize;
   const Confidence::Level MinConfidence;
   const VariableNamer::NamingStyle NamingStyle;
 };
 
 } // 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

StatementMatcher makePseudoArrayLoopMatcher() {
   // FIXME: To be completely thorough this matcher should also ensure the
   // 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(),
-                     hasDeclaration(cxxRecordDecl(
-                         hasMethod(cxxMethodDecl(hasName("begin"), isConst())),
-                         hasMethod(cxxMethodDecl(hasName("end"),
-                                                 isConst())))) // hasDeclaration
-                     ),                                        // qualType
-            qualType(unless(isConstQualified()),
-                     hasDeclaration(
-                         cxxRecordDecl(hasMethod(hasName("begin")),
+  TypeMatcher RecordWithBeginEnd = qualType(anyOf(
+      qualType(isConstQualified(),
+      qualType(
+          unless(isConstQualified()),
+          hasDeclaration(cxxRecordDecl(hasMethod(hasName("begin")),
                                        hasMethod(hasName("end"))))) // qualType
-            ));
 
   StatementMatcher SizeCallMatcher = cxxMemberCallExpr(
       argumentCountIs(0),
       callee(cxxMethodDecl(anyOf(hasName("size"), hasName("length")))),
       on(anyOf(hasType(pointsTo(RecordWithBeginEnd)),
                hasType(RecordWithBeginEnd))));
 
   StatementMatcher EndInitMatcher =

 }
 
 LoopConvertCheck::RangeDescriptor::RangeDescriptor()
     : ContainerNeedsDereference(false), DerefByConstRef(false),
       DerefByValue(false) {}
 
 LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
+      MaxCopySize(std::stoull(Options.get("MaxCopySize", "16"))),
       MinConfidence(StringSwitch<Confidence::Level>(
                         Options.get("MinConfidence", "reasonable"))
                         .Case("safe", Confidence::CL_Safe)
                         .Case("risky", Confidence::CL_Risky)
                         .Default(Confidence::CL_Reasonable)),
       NamingStyle(StringSwitch<VariableNamer::NamingStyle>(
                       Options.get("NamingStyle", "CamelCase"))
                       .Case("camelBack", VariableNamer::NS_CamelBack)
                       .Case("lower_case", VariableNamer::NS_LowerCase)
                       .Case("UPPER_CASE", VariableNamer::NS_UpperCase)
                       .Default(VariableNamer::NS_CamelCase)) {}
 
 void LoopConvertCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "MaxCopySize", std::to_string(MaxCopySize));
   SmallVector<std::string, 3> Confs{"risky", "reasonable", "safe"};
   Options.store(Opts, "MinConfidence", Confs[static_cast<int>(MinConfidence)]);
 
   SmallVector<std::string, 4> Styles{"camelBack", "CamelCase", "lower_case",
                                      "UPPER_CASE"};
   Options.store(Opts, "NamingStyle", Styles[static_cast<int>(NamingStyle)]);
 }
 
void LoopConvertCheck::doConversion(
 
   QualType Type = Context->getAutoDeductType();
   if (!Descriptor.ElemType.isNull() && Descriptor.ElemType->isFundamentalType())
     Type = Descriptor.ElemType.getUnqualifiedType();
 
   // 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.
-  bool IsTriviallyCopyable =
+  bool IsCheapToCopy =
       !Descriptor.ElemType.isNull() &&
-      Descriptor.ElemType.isTriviallyCopyableType(*Context);
+      Descriptor.ElemType.isTriviallyCopyableType(*Context) &&
+      // TypeInfo::Width is in bits.
+      Context->getTypeInfo(Descriptor.ElemType).Width <= 8 * MaxCopySize;
   bool UseCopy =
       CanCopy && ((VarNameFromAlias && !AliasVarIsRef) ||
-                  (Descriptor.DerefByConstRef && IsTriviallyCopyable));
+                  (Descriptor.DerefByConstRef && IsCheapToCopy));
 
   if (!UseCopy) {
     if (Descriptor.DerefByConstRef) {
       Type = Context->getLValueReferenceType(Context->getConstType(Type));
     } else if (Descriptor.DerefByValue) {
-      if (!IsTriviallyCopyable)
+      if (!IsCheapToCopy)
         Type = Context->getRValueReferenceType(Type);
     } else {
       Type = Context->getLValueReferenceType(Type);
     }
   }
 
   StringRef MaybeDereference = Descriptor.ContainerNeedsDereference ? "*" : "";
   std::string TypeString = Type.getAsString(getLangOpts());

clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp

void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<UseDefaultCheck>("modernize-use-default");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
   }
 
   ClangTidyOptions getModuleOptions() override {
     ClangTidyOptions Options;
     auto &Opts = Options.CheckOptions;
+    // For types whose size in bytes is above this threshold, we prefer taking a
+    // const-reference than making a copy.
+    Opts["modernize-loop-convert.MaxCopySize"] = "16";
+
     Opts["modernize-loop-convert.MinConfidence"] = "reasonable";
     Opts["modernize-loop-convert.NamingStyle"] = "CamelCase";
     Opts["modernize-pass-by-value.IncludeStyle"] = "llvm";    // Also: "google".
     Opts["modernize-replace-auto-ptr.IncludeStyle"] = "llvm"; // Also: "google".
 
     // Comma-separated list of macros that behave like NULL.
     Opts["modernize-use-nullptr.NullMacros"] = "NULL";
     return Options;

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

 
 struct NonTriviallyCopyable {
   NonTriviallyCopyable() = default;
   // Define this constructor to make this class non-trivially copyable.
   NonTriviallyCopyable(const NonTriviallyCopyable& Ntc);
   int X;
 };
 
+struct TriviallyCopyableButBig {
+  int X;
+  char Array[16];
+};
+
 struct S {
   typedef MutableVal *iterator;
   typedef const MutableVal *const_iterator;
   const_iterator begin() const;
   const_iterator end() const;
   const_iterator cbegin() const;
   const_iterator cend() const;
   iterator begin();

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

const int *constArray() {
   const NonTriviallyCopyable NonCopy[N]{};
   for (int I = 0; I < N; ++I) {
     printf("2 * %d = %d\n", NonCopy[I].X, NonCopy[I].X + NonCopy[I].X);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (const auto & Elem : NonCopy)
   // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", Elem.X, Elem.X + Elem.X);
 
+  const TriviallyCopyableButBig Big[N]{};
+  for (int I = 0; I < N; ++I) {
+    printf("2 * %d = %d\n", Big[I].X, Big[I].X + Big[I].X);
+  }
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (const auto & Elem : Big)
+  // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", Elem.X, Elem.X + Elem.X);
+
   bool Something = false;
   for (int I = 0; I < N; ++I) {
     if (Something)
       return &ConstArr[I];
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (const int & Elem : ConstArr)
   // CHECK-FIXES-NEXT: if (Something)