[NFC][clang] Fix static analyzer concerns

[Manna]

Summary

Reported by Coverity:

AUTO_CAUSES_COPY
Unnecessary object copies can affect performance.

1. Inside "SemaDeclCXX.cpp" file, in <unnamed>::​DiagnoseUninitializedFields(clang::​Sema &, clang::​CXXConstructorDecl const *): Using the auto keyword without an & causes the copy of an object of type CXXBaseSpecifier.

2. Inside "ClangAttrEmitter.cpp" file, in clang::​EmitClangAttrParsedAttrImpl(llvm::​RecordKeeper &, llvm::​raw_ostream &): Using the auto keyword without an & causes the copy of an object of type pair.

3. Inside "Marshallers.h" file, in clang::​ast_matchers::​dynamic::​internal::​MapAnyOfBuilderDescriptor::​buildMatcherCtor(clang::​ast_matchers::​dynamic::​SourceRange, llvm::​ArrayRef<clang::​ast_matchers::​dynamic::​ParserValue>, clang::​ast_matchers::​dynamic::​Diagnostics *): Using the auto keyword without an & causes the copy of an object of type ParserValue.

4. Inside "CGVTables.cpp" file, in clang::​CodeGen::​CodeGenModule::​GetVCallVisibilityLevel(clang::​CXXRecordDecl const *, llvm::​DenseSet<clang::​CXXRecordDecl const *, llvm::​DenseMapInfo<clang::​CXXRecordDecl const *, void>> &): Using the auto keyword without an & causes the copy of an object of type CXXBaseSpecifier.

5. Inside "ASTContext.cpp" file, in hasTemplateSpecializationInEncodedString(clang::​Type const *, bool): Using the auto keyword without an & causes the copy of an object of type CXXBaseSpecifier.

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

[Manna]

clang/lib/AST/ASTContext.cpp

8206

CXXBaseSpecifier is less in size, but most of cases we are using reference semantics.

clang/lib/AST/ComputeDependence.cpp

763

TemplateArgumentLoc has a TemplateArgument (pass by value usually), but it also has a TemplateArgumentLocInfo, which is a pair of pointers + source locations.

clang/lib/ASTMatchers/Dynamic/Marshallers.h

1011

This returns ParserValue which passes as a reference.

clang/lib/CodeGen/CGVTables.cpp

1278

CXXBaseSpecifier is less in size, but most of cases we are using reference semantics.

clang/lib/Lex/Pragma.cpp

1110 ↗

(On Diff #515355)

This returns a std::pair<IdentifierInfo *, SourceLocation> which is not particularly expensive to copy

clang/lib/Sema/SemaDeclCXX.cpp

3974

CXXBaseSpecifier is less in size, but most of cases we are using reference semantics.

clang/utils/TableGen/ClangAttrEmitter.cpp

4262

Passes as a reference

[erichkeane]

clang/lib/AST/ASTContext.cpp

8206

A bit bigger than 2 pointers actually, its ~1 byte in bitfields, 12 bytes in SourceLocation, and a pointer.

So perhaps still on the line. However, it is only dereferenced 1x.

clang/lib/Lex/Pragma.cpp

1110 ↗

(On Diff #515355)

In that case, I'd skip the change here and leave it as a copy.

clang/utils/TableGen/ClangAttrEmitter.cpp

4262

Each element is a pair of a std::string and const Record*, which should justify this. Not sure what you mean by 'passes as a reference'?

clang/utils/TableGen/RISCVVEmitter.cpp

562 ↗

(On Diff #515355)

What type is 'P' here? What size is it? It is being used quite a few times in this loop, which means it would have to be pretty sizable to justify making it a reference unless it has a copy ctor of cost.

[tahonermann]

With the exception of the case involving the Policy class, these changes all look fine to me.

clang/utils/TableGen/RISCVVEmitter.cpp

562 ↗

(On Diff #515355)

P is class Policy from clang/include/clang/Support/RISCVVIntrinsicUtils.h. It holds two enum values (with non-fixed underlying type) and has an implicit copy constructor, so cheap to copy. I suspect the code gen impact is negligible here, but I see no need for reference semantics. I recommend skipping this change.

[Manna]

I have removed the case involving the Policy class

[tahonermann]

Thanks, Soumi! Looks good to me!

[Manna]

Thank you @tahonermann and @erichkeane for reviews and feedback.

clang/utils/TableGen/ClangAttrEmitter.cpp

4262

Each element is a pair of a std::string and const Record*, which should justify this.

Sorry i meant to say this instead since ParsedAttrMap Attrs = getParsedAttrList(Records, &Dupes);

[Manna]

@erichkeane do you have any more concerns?

[erichkeane]

Still had this convo which wasn't resolved.

clang/lib/Lex/Pragma.cpp

1110 ↗

(On Diff #515355)

This seems like it should be a copy. About a pointer-and-a-half with a cheap ctor.

[Manna]

I have addressed @erichkeane's review comment.

[Manna]

clang/lib/Lex/Pragma.cpp

1110 ↗

(On Diff #515355)

I missed that. Thanks @erichkeane. Done

clang/lib/AST/ASTContext.cpp

if (!CXXRD)
     return false;
 
   if (isa<ClassTemplateSpecializationDecl>(CXXRD))
     return true;
 
   if (!CXXRD->hasDefinition() || !VisitBasesAndFields)
     return false;
 
-  for (auto B : CXXRD->bases())
+  for (const auto &B : CXXRD->bases())
     if (hasTemplateSpecializationInEncodedString(B.getType().getTypePtr(),
                                                  true))
       return true;
 
   for (auto *FD : CXXRD->fields())
     if (hasTemplateSpecializationInEncodedString(FD->getType().getTypePtr(),
                                                  true))
       return true;

clang/lib/AST/ComputeDependence.cpp

clang::computeDependence(OverloadExpr *E, bool KnownDependent,
   return Deps;
 }
 
 ExprDependence clang::computeDependence(DependentScopeDeclRefExpr *E) {
   auto D = ExprDependence::TypeValue;
   D |= getDependenceInExpr(E->getNameInfo());
   if (auto *Q = E->getQualifier())
     D |= toExprDependence(Q->getDependence());
-  for (auto A : E->template_arguments())
+  for (const auto &A : E->template_arguments())
     D |= toExprDependence(A.getArgument().getDependence());
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXConstructExpr *E) {
   ExprDependence D =
       toExprDependenceForImpliedType(E->getType()->getDependence());
   for (auto *A : E->arguments())

 
 ExprDependence clang::computeDependence(CXXDependentScopeMemberExpr *E) {
   auto D = ExprDependence::TypeValueInstantiation;
   if (!E->isImplicitAccess())
     D |= E->getBase()->getDependence();
   if (auto *Q = E->getQualifier())
     D |= toExprDependence(Q->getDependence());
   D |= getDependenceInExpr(E->getMemberNameInfo());
-  for (auto A : E->template_arguments())
+  for (const auto &A : E->template_arguments())
     D |= toExprDependence(A.getArgument().getDependence());
   return D;
 }
 
 ExprDependence clang::computeDependence(MaterializeTemporaryExpr *E) {
   return E->getSubExpr()->getDependence();
 }
 

clang/lib/ASTMatchers/Dynamic/Marshallers.h

public:
 
   bool isBuilderMatcher() const override { return true; }
 
   std::unique_ptr<MatcherDescriptor>
   buildMatcherCtor(SourceRange, ArrayRef<ParserValue> Args,
                    Diagnostics *) const override {
 
     std::vector<ASTNodeKind> NodeKinds;
-    for (auto Arg : Args) {
+    for (const auto &Arg : Args) {
       if (!Arg.Value.isNodeKind())
         return {};
       NodeKinds.push_back(Arg.Value.getNodeKind());
     }
 
     if (NodeKinds.empty())
       return {};
 

clang/lib/CodeGen/CGVTables.cpp

llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
   llvm::GlobalObject::VCallVisibility TypeVis;
   if (!isExternallyVisible(LV.getLinkage()))
     TypeVis = llvm::GlobalObject::VCallVisibilityTranslationUnit;
   else if (HasHiddenLTOVisibility(RD))
     TypeVis = llvm::GlobalObject::VCallVisibilityLinkageUnit;
   else
     TypeVis = llvm::GlobalObject::VCallVisibilityPublic;
 
-  for (auto B : RD->bases())
+  for (const auto &B : RD->bases())
     if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
       TypeVis = std::min(
           TypeVis,
           GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
 
-  for (auto B : RD->vbases())
+  for (const auto &B : RD->vbases())
     if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
       TypeVis = std::min(
           TypeVis,
           GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
 
   return TypeVis;
 }
 

clang/lib/Sema/SemaDeclCXX.cpp

for (auto *I : RD->decls()) {
       if (auto *FD = dyn_cast<FieldDecl>(I)) {
         UninitializedFields.insert(FD);
       } else if (auto *IFD = dyn_cast<IndirectFieldDecl>(I)) {
         UninitializedFields.insert(IFD->getAnonField());
       }
     }
 
     llvm::SmallPtrSet<QualType, 4> UninitializedBaseClasses;
-    for (auto I : RD->bases())
+    for (const auto &I : RD->bases())
       UninitializedBaseClasses.insert(I.getType().getCanonicalType());
 
     if (UninitializedFields.empty() && UninitializedBaseClasses.empty())
       return;
 
     UninitializedFieldVisitor UninitializedChecker(SemaRef,
                                                    UninitializedFields,
                                                    UninitializedBaseClasses);

clang/utils/TableGen/ClangAttrEmitter.cpp

void EmitClangAttrParsedAttrImpl(RecordKeeper &Records, raw_ostream &OS) {
 
   // Get the list of parsed attributes, and accept the optional list of
   // duplicates due to the ParseKind.
   ParsedAttrMap Dupes;
   ParsedAttrMap Attrs = getParsedAttrList(Records, &Dupes);
 
   // Generate all of the custom appertainsTo functions that the attributes
   // will be using.
-  for (auto I : Attrs) {
+  for (const auto &I : Attrs) {
     const Record &Attr = *I.second;
     if (Attr.isValueUnset("Subjects"))
       continue;
     const Record *SubjectObj = Attr.getValueAsDef("Subjects");
     for (auto Subject : SubjectObj->getValueAsListOfDefs("Subjects"))
       if (Subject->isSubClassOf("SubsetSubject"))
         GenerateCustomAppertainsTo(*Subject, OS);
   }