[PATCH] D148639: [NFC][clang] Fix static analyzer concerns about AUTO_CAUSES_COPY

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.



================
Comment at: clang/lib/AST/ExprConstant.cpp:6117
       } else
-        for (auto Idx : Attr->args()) {
+        for (const auto &Idx : Attr->args()) {
           unsigned ASTIdx = Idx.getASTIndex();
----------------
This returns a `ParamIdx` object which is 32-bits, so it's not expensive to copy.


================
Comment at: clang/lib/Basic/TargetID.cpp:136
     OrderedMap[F.first()] = F.second;
-  for (auto F : OrderedMap)
+  for (const auto &F : OrderedMap)
     TargetID = TargetID + ':' + F.first.str() + (F.second ? "+" : "-");
----------------
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.


================
Comment at: clang/lib/Basic/Targets/ARM.cpp:463
 
-  for (auto Feature : TargetFeatures)
+  for (const auto &Feature : TargetFeatures)
     if (Feature[0] == '+')
----------------
This returns a `StringRef` which is not expensive to copy.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3548
   SmallVector<std::pair<llvm::Value *, SanitizerMask>, 5> Checks;
-  for (auto CheckKindMaskPair : CheckKinds) {
+  for (const auto &CheckKindMaskPair : CheckKinds) {
     int Kind = CheckKindMaskPair.first;
----------------
This is a pair of ints, not expensive to copy.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9426
       } else {
         for (auto IvarPair : DuplicateIvars) {
           Diag(IvarPair.first->getLocation(),
----------------
Why is the above considered too expensive but this one is not?


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:989
   std::vector<FileID> Keys;
-  for (auto F : Files)
+  for (const auto &F : Files)
     Keys.push_back(F.first);
----------------
`FileID` is an `int`, not expensive to copy.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148639/new/

https://reviews.llvm.org/D148639