[NFC][clang] Fix Coverity bugs with AUTO_CAUSES_COPY

[Manna]

Summary

Reported by Coverity:
AUTO_CAUSES_COPY
Unnecessary object copies can affect performance.

1. Inside "ExtractAPIVisitor.h" file, in clang::​extractapi::​impl::​ExtractAPIVisitorBase<<unnamed>::​BatchExtractAPIVisitor>::​VisitFunctionDecl(clang::​FunctionDecl const *): Using the auto keyword without an & causes the copy of an object of type DynTypedNode.

2. Inside "NeonEmitter.cpp" file, in <unnamed>::​Intrinsic::​Intrinsic(llvm::​Record *, llvm::​StringRef, llvm::​StringRef, <unnamed>::​TypeSpec, <unnamed>::​TypeSpec, <unnamed>::​ClassKind, llvm::​ListInit *, <unnamed>::​NeonEmitter &, llvm::​StringRef, llvm::​StringRef, bool, bool): Using the auto keyword without an & causes the copy of an object of type Type.

3. Inside "MicrosoftCXXABI.cpp" file, in <unnamed>::​MSRTTIBuilder::​getClassHierarchyDescriptor(): Using the auto keyword without an & causes the copy of an object of type MSRTTIClass.

4. Inside "CGGPUBuiltin.cpp" file, in clang::​CodeGen::​CodeGenFunction::​EmitAMDGPUDevicePrintfCallExpr(clang::​CallExpr const *): Using the auto keyword without an & causes the copy of an object of type CallArg.

5. Inside "SemaDeclAttr.cpp" file, in threadSafetyCheckIsSmartPointer(clang::​Sema &, clang::​RecordType const *): Using the auto keyword without an & causes the copy of an object of type CXXBaseSpecifier.

6. Inside "ComputeDependence.cpp" file, in clang::​computeDependence(clang::​DesignatedInitExpr *): Using the auto keyword without an & causes the copy of an object of type Designator.

7. Inside "Format.cpp" file, In clang::​format::​affectsRange(llvm::​ArrayRef<clang::​tooling::​Range>, unsigned int, unsigned int): Using the auto keyword without an & causes the copy of an object of type Range.

[tahonermann]

I added a comment to skip the one where the element type is a simple pair. The rest look fine to me.

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h

182

I think this is probably a good change. getParents() returns a DynTypedNodeList (clang/include/clang/AST/ParentMapContext.h) that uses DynTypedNode as its element type. DynTypedNode (clang/include/clang/AST/ASTTypeTraits.h) has a llvm::AlignedCharArrayUnion data member that looks like it is sufficiently complicated that avoiding copies is probably advantageous.

clang/lib/AST/ComputeDependence.cpp

666

The element type here is Designator (clang/include/clang/AST/Expr.h). It functions as a discriminated union; its largest union member has three source locations and an unsigned value. Copy constructors look to be trivial, but still, this seems like a win.

clang/lib/CodeGen/CGGPUBuiltin.cpp

192

The element type is CallArg (clang/lib/CodeGen/CGCall.h). This looks like a clear win; CallArg is another discriminated union and its LValue member holds a number of pointer members.

clang/lib/CodeGen/MicrosoftCXXABI.cpp

3760

MSRTTIClass (clang/lib/CodeGen/MicrosoftCXXABI.cpp) holds two pointers and three uint32_t values. Seems like a reasonable change to me.

clang/lib/Format/Format.cpp

2809

The element type is Range (clang/include/clang/Tooling/Core/Replacement.h) and it just holds two unsigned values. I don't think this change is needed (though I don't think it would hurt anything either).

clang/lib/Sema/SemaDeclAttr.cpp

450

The element type is CXXBaseSpecifier (clang/include/clang/AST/DeclCXX.h). This change seems fine to me.

clang/lib/Sema/SemaLookup.cpp

4662 ↗

(On Diff #516445)

The element type is a pair of NamespaceDecl * and bool. I would skip this change.

clang/utils/TableGen/NeonEmitter.cpp

392

The element type is Type (clang/utils/TableGen/NeonEmitter.cpp). It holds a TypeSpec, an enum value, 5 bool values, and 3 unsigned values. I'm ambivalent.

[Manna]

I have addressed @tahonermann review comments.

clang/utils/TableGen/NeonEmitter.cpp

392

Thanks @tahonermann for reviews and feedbacks.

The element type is Type (clang/utils/TableGen/NeonEmitter.cpp). It holds a TypeSpec, an enum value, 5 bool values, and 3 unsigned values.

Yes, I had mixed feeling about this one too.

Line 362 in clang/utils/TableGen/NeonEmitter.cpp, we are passing it as a reference.

for (const auto &T : Types){

    if (T.isVector() && T.getNumElements() > 1)
      return false;
  }
  return true;
}

[Manna]

clang/lib/Sema/SemaLookup.cpp

4662 ↗

(On Diff #516445)

Thanks @tahonermann for the explanation. Removed

[tahonermann]

Looks good to me, thanks @Manna!

clang/utils/TableGen/NeonEmitter.cpp

392

Consistency is good then.

[Manna]

In D149074#4293543, @tahonermann wrote:

Looks good to me

Thank you @tahonermann

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h

 bool ExtractAPIVisitorBase<Derived>::VisitFunctionDecl(
     const FunctionDecl *Decl) {
   if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) {
     // Skip member function in class templates.
     if (Method->getParent()->getDescribedClassTemplate() != nullptr)
       return true;
 
     // Skip methods in records.
-    for (auto P : Context.getParents(*Method)) {
+    for (const auto &P : Context.getParents(*Method)) {
       if (P.template get<CXXRecordDecl>())
         return true;
     }
 
     // Skip ConstructorDecl and DestructorDecl.
     if (isa<CXXConstructorDecl>(Method) || isa<CXXDestructorDecl>(Method))
       return true;
   }

clang/lib/AST/ComputeDependence.cpp

ExprDependence clang::computeDependence(GenericSelectionExpr *E,
   if (E->isResultDependent())
     return D | ExprDependence::TypeValueInstantiation;
   return D | (E->getResultExpr()->getDependence() &
               ~ExprDependence::UnexpandedPack);
 }
 
 ExprDependence clang::computeDependence(DesignatedInitExpr *E) {
   auto Deps = E->getInit()->getDependence();
-  for (auto D : E->designators()) {
+  for (const auto &D : E->designators()) {
     auto DesignatorDeps = ExprDependence::None;
     if (D.isArrayDesignator())
       DesignatorDeps |= E->getArrayIndex(D)->getDependence();
     else if (D.isArrayRangeDesignator())
       DesignatorDeps |= E->getArrayRangeStart(D)->getDependence() |
                         E->getArrayRangeEnd(D)->getDependence();
     Deps |= DesignatorDeps;
     if (DesignatorDeps & ExprDependence::TypeValue)

clang/lib/CodeGen/CGGPUBuiltin.cpp

RValue CodeGenFunction::EmitAMDGPUDevicePrintfCallExpr(const CallExpr *E) {
 
   CallArgList CallArgs;
   EmitCallArgs(CallArgs,
                E->getDirectCallee()->getType()->getAs<FunctionProtoType>(),
                E->arguments(), E->getDirectCallee(),
                /* ParamsToSkip = */ 0);
 
   SmallVector<llvm::Value *, 8> Args;
-  for (auto A : CallArgs) {
+  for (const auto &A : CallArgs) {
     // We don't know how to emit non-scalar varargs.
     if (!A.getRValue(*this).isScalar()) {
       CGM.ErrorUnsupported(E, "non-scalar arg to printf");
       return RValue::get(llvm::ConstantInt::get(IntTy, -1));
     }
 
     llvm::Value *Arg = A.getRValue(*this).getScalarVal();
     Args.push_back(Arg);

clang/lib/CodeGen/MicrosoftCXXABI.cpp

if (auto CHD = Module.getNamedGlobal(MangledName))
     return CHD;
 
   // Serialize the class hierarchy and initialize the CHD Fields.
   SmallVector<MSRTTIClass, 8> Classes;
   serializeClassHierarchy(Classes, RD);
   Classes.front().initialize(/*Parent=*/nullptr, /*Specifier=*/nullptr);
   detectAmbiguousBases(Classes);
   int Flags = 0;
-  for (auto Class : Classes) {
+  for (const MSRTTIClass &Class : Classes) {
     if (Class.RD->getNumBases() > 1)
       Flags |= HasBranchingHierarchy;
     // Note: cl.exe does not calculate "HasAmbiguousBases" correctly.  We
     // believe the field isn't actually used.
     if (Class.Flags & MSRTTIClass::IsAmbiguous)
       Flags |= HasAmbiguousBases;
   }
   if ((Flags & HasBranchingHierarchy) && RD->getNumVBases() != 0)

clang/lib/Format/Format.cpp

struct JavaImportDirective {
   bool IsStatic;
 };
 
 } // end anonymous namespace
 
 // Determines whether 'Ranges' intersects with ('Start', 'End').
 static bool affectsRange(ArrayRef<tooling::Range> Ranges, unsigned Start,
                          unsigned End) {
-  for (auto Range : Ranges) {
+  for (const auto &Range : Ranges) {
     if (Range.getOffset() < End &&
         Range.getOffset() + Range.getLength() > Start) {
       return true;
     }
   }
   return false;
 }
 

clang/lib/Sema/SemaDeclAttr.cpp

static bool threadSafetyCheckIsSmartPointer(Sema &S, const RecordType* RT) {
   bool foundArrowOperator = IsOverloadedOperatorPresent(Record, OO_Arrow);
   if (foundStarOperator && foundArrowOperator)
     return true;
 
   const CXXRecordDecl *CXXRecord = dyn_cast<CXXRecordDecl>(Record);
   if (!CXXRecord)
     return false;
 
-  for (auto BaseSpecifier : CXXRecord->bases()) {
+  for (const auto &BaseSpecifier : CXXRecord->bases()) {
     if (!foundStarOperator)
       foundStarOperator = IsOverloadedOperatorPresent(
           BaseSpecifier.getType()->getAsRecordDecl(), OO_Star);
     if (!foundArrowOperator)
       foundArrowOperator = IsOverloadedOperatorPresent(
           BaseSpecifier.getType()->getAsRecordDecl(), OO_Arrow);
   }
 

clang/utils/TableGen/NeonEmitter.cpp

Intrinsic(Record *R, StringRef Name, StringRef Proto, TypeSpec OutTS,
     while (!Mods.empty()) {
       Types.emplace_back(InTS, Mods);
       if (Mods.contains('!'))
         PolymorphicKeyType = Types.size() - 1;
 
       Mods = getNextModifiers(Proto, Pos);
     }
 
-    for (auto Type : Types) {
+    for (const auto &Type : Types) {
       // If this builtin takes an immediate argument, we need to #define it rather
       // than use a standard declaration, so that SemaChecking can range check
       // the immediate passed by the user.
 
       // Pointer arguments need to use macros to avoid hiding aligned attributes
       // from the pointer type.
 
       // It is not permitted to pass or return an __fp16 by value, so intrinsics