[NFC][clang] Fix static analyzer concerns about AUTO_CAUSES_COPY

[Manna]

Summary

Reported by Coverity:

AUTO_CAUSES_COPY
Unnecessary object copies can affect performance

1. Inside "ODRHash.cpp" file, in clang::​ODRHash::​AddCXXRecordDecl(clang::​CXXRecordDecl const *): Using the auto keyword without an & causes the copy of an object of type CXXBaseSpecifier.

2. Inside "Tokens.cpp" file, in clang::​syntax::​TokenBuffer::​dumpForTests[abi:cxx11](): Using the auto keyword without an & causes the copy of an object of type DenseMapPair.

3. Inside "TargetID.cpp" file, in clang::​getCanonicalTargetID[abi:cxx11](llvm::​StringRef, llvm::​StringMap<bool, llvm::​MallocAllocator> const &): Using the auto keyword without an & causes the copy of an object of type pair.

[aaron.ballman]

Thanks for the static analysis fixes, however, it looks like quite a few of them were false positives again. When fixing any static analysis diagnostics, please put in the effort to determine whether it's actually correct or not, as it seem the Coverity diagnostic is not really paying any attention to the size of what's being copied. If you're applying the advice from the tool without further analysis, it can be disruptive (might introduce bugs, churns the code for negative/no benefit, lowers reviewer confidence in the rest of the changes, etc). (Not suggesting you're blindly applying advice, but mostly observing we've had a few reviews like this.)

My suggestion is to always look at the size of what's being copied. As a general rule of thumb, if it's two pointers or less in size, the copy is likely cheaper than the reference and shouldn't be changed.

clang/lib/AST/ExprConstant.cpp

6117 ↗

(On Diff #514643)

This returns a ParamIdx object which is 32-bits, so it's not expensive to copy.

clang/lib/Basic/TargetID.cpp

136

The map iterator returns a std::pair<StringRef, bool> which is not particularly expensive to copy, but I think this is borderline enough that it's fine to keep.

clang/lib/Basic/Targets/ARM.cpp

463 ↗

(On Diff #514643)

This returns a StringRef which is not expensive to copy.

clang/lib/CodeGen/CGExpr.cpp

3548 ↗

(On Diff #514643)

This is a pair of ints, not expensive to copy.

clang/lib/Serialization/ASTReader.cpp

9426 ↗

(On Diff #514643)

Why is the above considered too expensive but this one is not?

clang/lib/Tooling/Syntax/Tokens.cpp

989

FileID is an int, not expensive to copy.

[tahonermann]

After seeing this review, I filed a support case with Synopsys with a request to stop reporting AUTO_CAUSES_COPY issues for small objects of class types, at least in the absence of additional evidence that a reference would be preferred.

clang/lib/AST/ODRHash.cpp

597

This seems like a good change; CXXBaseSpecifier isn't particularly large, but this seems like a case where reference semantics are more intuitive anyway.

clang/lib/Serialization/ASTReader.cpp

9426 ↗

(On Diff #514643)

I'm guessing that Coverity reported it as a second occurrence and those are easy to overlook.

clang/lib/Tooling/Syntax/Tokens.cpp

989

Files has type llvm::DenseMap<FileID, MarkedFile>, so the element type is DenseMapPair<FileID, MarkedFile>. MarkedFile contains a couple of vectors, so this looks like a win to me.

[steakhal]

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1053 ↗

(On Diff #514643)

I think this is supposed to be just some lightweight handle: some discriminator and pointer under the hood. sizeof(I)
is just 40 bytes. Another point is that the copy construction is not customized, so the sizeof for such objects should be a good proxy for estimating how costly the copy is.

For me personally, if it fits into a cacheline (on x86_64 I think its 64 bytes) (and ~trivially-copyable) then it should've probably taken by value.
I haven't measured this theory, so take it with a pinch of salt.

[Manna]

Thank you everyone for reviews and feedbacks. I misunderstood some of the False positive Coverity cases. Thanks for the explanation and suggestions. I will keep this in mind while investigating static analysis in future. This patch removes FP cases.

[Manna]

clang/lib/Serialization/ASTReader.cpp

9426 ↗

(On Diff #514643)

Yes, i missed this case. I will investigate it further, so this case has been removed from the current patch.

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp

1053 ↗

(On Diff #514643)

Thanks for the explanation!

[tahonermann]

This set of changes looks good to me.

[Manna]

@aaron.ballman, do you have any more concerns or comments? Thank you

[Manna]

In D148639#4284085, @tahonermann wrote:

This set of changes looks good to me.

Thank you @tahonermann

clang/lib/AST/ODRHash.cpp

void ODRHash::AddCXXRecordDecl(const CXXRecordDecl *Record) {
   const ClassTemplateDecl *TD = Record->getDescribedClassTemplate();
   AddBoolean(TD);
   if (TD) {
     AddTemplateParameterList(TD->getTemplateParameters());
   }
 
   ID.AddInteger(Record->getNumBases());
   auto Bases = Record->bases();
-  for (auto Base : Bases) {
+  for (const auto &Base : Bases) {
     AddQualType(Base.getType());
     ID.AddInteger(Base.isVirtual());
     ID.AddInteger(Base.getAccessSpecifierAsWritten());
   }
 }
 
 void ODRHash::AddRecordDecl(const RecordDecl *Record) {
   assert(!isa<CXXRecordDecl>(Record) &&

clang/lib/Basic/TargetID.cpp

 // A canonical target ID is a target ID containing a canonical processor name
 // and features in alphabetical order.
 std::string getCanonicalTargetID(llvm::StringRef Processor,
                                  const llvm::StringMap<bool> &Features) {
   std::string TargetID = Processor.str();
   std::map<const llvm::StringRef, bool> OrderedMap;
   for (const auto &F : Features)
     OrderedMap[F.first()] = F.second;
-  for (auto F : OrderedMap)
+  for (const auto &F : OrderedMap)
     TargetID = TargetID + ':' + F.first.str() + (F.second ? "+" : "-");
   return TargetID;
 }
 
 // For a specific processor, a feature either shows up in all target IDs, or
 // does not show up in any target IDs. Otherwise the target ID combination
 // is invalid.
 std::optional<std::pair<llvm::StringRef, llvm::StringRef>>

clang/lib/Tooling/Syntax/Tokens.cpp

std::string TokenBuffer::dumpForTests() const {
 
   OS << "expanded tokens:\n"
      << "  ";
   // (!) we do not show '<eof>'.
   DumpTokens(OS, llvm::ArrayRef(ExpandedTokens).drop_back());
   OS << "\n";
 
   std::vector<FileID> Keys;
-  for (auto F : Files)
+  for (const auto &F : Files)
     Keys.push_back(F.first);
   llvm::sort(Keys);
 
   for (FileID ID : Keys) {
     const MarkedFile &File = Files.find(ID)->second;
     auto *Entry = SourceMgr->getFileEntryForID(ID);
     if (!Entry)
       continue; // Skip builtin files.