[NFC][clang] Fix Coverity static analyzer tool concerns about auto_causes_copy

[Manna]

Summary

Reported by Coverity:

AUTO_CAUSES_COPY
Unnecessary object copies can affect performance

1. Inside FrontendActions.cpp file,

In clang::​DumpModuleInfoAction::​ExecuteAction(): Using the auto keyword without an & causes the copy of an object of type pair.

2. Inside ComputeDependence.cpp file,

In clang::​computeDependence(clang::​OverloadExpr *, bool, bool, bool): Using the auto keyword without an & causes the copy of an object of type TemplateArgumentLoc.

[erichkeane]

Did an analysis over each of these, only 2 I think needs to not be made, 2 are questionable, other one looks fine. I'll leave it to @aaron.ballman/others to interpret my thoughts below.

clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h

89

So succs and pred are both of type AdjacentBlock. This type is 2 ptrs long, which is roughly the 'decision point' for me. That is, a 2 pointer copy is cheap, and forming a reference requires a series of indirections, so I could go either way on this one. However, in both cases, only a single indirection is done (since enqueueBlock is being called with a CFGBlock via the conversion operator), so the indirection cost is low.

So I think these are OK to me.

clang/lib/AST/ComputeDependence.cpp

753

Ends up being pretty sizable, TemplateArgumentLoc has a TemplateArgument (which we typically pass around by value), but it also has a TemplateArgumentLocInfo, which is a pair of pointers + a pair of source locations.

clang/lib/AST/Type.cpp

748

So QualType is usually a type we pass around by value, since it is a pointer/int pair. So the copy here is intentional IMO.

clang/lib/Frontend/FrontendActions.cpp

885

A pair of a string and the other type, definitely worth saving the copies.

clang/lib/Sema/SemaCoroutine.cpp

1149

This ends up being a pair of pointers(ParmVarDecl* and Stmt*). so again I'm on the fence here :/ Probably OK doing anyway, since there is again only a single indirection, so the change is probably a wash.

clang/lib/Sema/SemaDeclCXX.cpp

804

This is just a SourceLocation, so it is tiny. Definitely not worth doing a reference here.

[Manna]

Thanks @erichkeane for reviews and feedbacks.

[aaron.ballman]

In general, I agree with the comments from @erichkeane. We don't typically want to pass around const references to tiny objects, so QualType and SourceLocation uses should be left alone. The changes in ComputeDependence.cpp and FrontendActions.cpp seem reasonably well-motivated, but the rest feel like changes just to silence a static analyzer diagnostic. In general, I think we usually take the view that these sort of speculative perf changes aren't worth the churn unless there's a measurable performance issue being addressed. Did you do a performance analysis and find that there were concerns in any of these places? If not, then I'd say we should take just the two changes and drop the rest. WDYT?

[Manna]

Thanks @aaron.ballman for reviews. I have updated patch.

[erichkeane]

Hi Soumi-
I think you've managed to do the inverse of what we suggested, at least that is how it looks! Aaron suggested keeping: ComputeDependence.cpp and FrontendActions.cpp, and not doing the rest.

[Manna]

Thanks @erichkeane and @aaron.ballman for the reviews and feedbacks. I have updated diff that contains changes from ComputeDependence.cpp and FrontendActions.cpp files.

[Manna]

In D147552#4244313, @erichkeane wrote:

Hi Soumi-
I think you've managed to do the inverse of what we suggested, at least that is how it looks! Aaron suggested keeping: ComputeDependence.cpp and FrontendActions.cpp, and not doing the rest.

Sorry @erichkeane. I uploaded wrong patch.

[Manna]

I have created https://reviews.llvm.org/D147574

clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h

struct BackwardDataflowWorklist
     : DataflowWorklistBase<PostOrderCFGView::BlockOrderCompare, 20> {
   BackwardDataflowWorklist(const CFG &Cfg, AnalysisDeclContext &Ctx)
       : DataflowWorklistBase(
             Cfg, Ctx.getAnalysis<PostOrderCFGView>(),
             Ctx.getAnalysis<PostOrderCFGView>()->getComparator()) {}
 
   void enqueuePredecessors(const CFGBlock *Block) {
     for (auto B : Block->preds())
       enqueueBlock(B);
   }
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_DATAFLOWWORKLIST_H

clang/lib/AST/ComputeDependence.cpp

clang::computeDependence(OverloadExpr *E, bool KnownDependent,
   for (auto *D : E->decls()) {
     if (D->getDeclContext()->isDependentContext() ||
         isa<UnresolvedUsingValueDecl>(D))
       Deps |= ExprDependence::TypeValueInstantiation;
   }
   // If we have explicit template arguments, check for dependent
   // template arguments and whether they contain any unexpanded pack
   // expansions.
-  for (auto A : E->template_arguments())
+  for (const auto &A : E->template_arguments())
     Deps |= toExprDependence(A.getArgument().getDependence());
   return Deps;
 }
 
 ExprDependence clang::computeDependence(DependentScopeDeclRefExpr *E) {
   auto D = ExprDependence::TypeValue;
   D |= getDependenceInExpr(E->getNameInfo());
   if (auto *Q = E->getQualifier())

clang/lib/AST/Type.cpp

ObjCObjectType::ObjCObjectType(QualType Canonical, QualType Base,
 
   ObjCObjectTypeBits.NumTypeArgs = typeArgs.size();
   assert(getTypeArgsAsWritten().size() == typeArgs.size() &&
          "bitfield overflow in type argument count");
   if (!typeArgs.empty())
     memcpy(getTypeArgStorage(), typeArgs.data(),
            typeArgs.size() * sizeof(QualType));
 
   for (auto typeArg : typeArgs) {
     addDependence(typeArg->getDependence() & ~TypeDependence::VariablyModified);
   }
   // Initialize the protocol qualifiers. The protocol storage is known
   // after we set number of type arguments.
   initialize(protocols);
 }
 
 bool ObjCObjectType::isSpecialized() const {

clang/lib/Frontend/FrontendActions.cpp

if (auto FilteredMacros = llvm::make_filter_range(
         !FilteredMacros.empty()) {
       Out << "   Macro Definitions:\n";
       for (/*<IdentifierInfo *, MacroState> pair*/ const auto &Macro :
            FilteredMacros)
         Out << "     " << Macro.first->getName() << "\n";
     }
 
     // Now let's print out any modules we did not see as part of the Primary.
-    for (auto SM : SubModMap) {
+    for (const auto &SM : SubModMap) {
       if (!SM.second.Seen && SM.second.Mod) {
         Out << "  " << ModuleKindName(SM.second.Kind) << " '" << SM.first
             << "' at index #" << SM.second.Idx
             << " has no direct reference in the Primary\n";
       }
     }
     Out << "  ====== ======\n";
   }

clang/lib/Sema/SemaCoroutine.cpp

CoroutineStmtBuilder::CoroutineStmtBuilder(Sema &S, FunctionDecl &FD,
                                            sema::FunctionScopeInfo &Fn,
                                            Stmt *Body)
     : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
       IsPromiseDependentType(
           !Fn.CoroutinePromise ||
           Fn.CoroutinePromise->getType()->isDependentType()) {
   this->Body = Body;
 
   for (auto KV : Fn.CoroutineParameterMoves)
     this->ParamMovesVector.push_back(KV.second);
   this->ParamMoves = this->ParamMovesVector;
 
   if (!IsPromiseDependentType) {
     PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
     assert(PromiseRecordDecl && "Type should have already been checked");
   }
   this->IsValid = makePromiseStmt() && makeInitialAndFinalSuspend();

clang/lib/Sema/SemaDeclCXX.cpp

auto &DS = D.getDeclSpec();
     }
 
     if (!BadSpecifiers.empty()) {
       auto &&Err = Diag(BadSpecifierLocs.front(), diag::err_decomp_decl_spec);
       Err << (int)BadSpecifiers.size()
           << llvm::join(BadSpecifiers.begin(), BadSpecifiers.end(), " ");
       // Don't add FixItHints to remove the specifiers; we do still respect
       // them when building the underlying variable.
       for (auto Loc : BadSpecifierLocs)
         Err << SourceRange(Loc, Loc);
     } else if (!CPlusPlus20Specifiers.empty()) {
       auto &&Warn = Diag(CPlusPlus20SpecifierLocs.front(),
                          getLangOpts().CPlusPlus20
                              ? diag::warn_cxx17_compat_decomp_decl_spec
                              : diag::ext_decomp_decl_spec);
       Warn << (int)CPlusPlus20Specifiers.size()
            << llvm::join(CPlusPlus20Specifiers.begin(),