[PATCH] D157129: [NFC] Fix unnecessary copy with auto.

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

I went over the patch and I found only a single debatable case for taking by reference.
To clarify why I would prefer taking by value: value semantics is good for local reasoning; thus improves maintainability. We should only deviate from that when there is actual benefit for doing so.
Static analysis tools, such as Coverity, have false-positives. Some rules are better than others.
As a static analysis tool developer myself, I'd recommend carefully evaluating the findings before taking action for resolving them.
And if you find anything interesting, tool vendors are generally happy to receive feedback about their tool. I guess, they should as well understand that taking a pointer by reference doesn't improve anything.



================
Comment at: clang/include/clang/Analysis/FlowSensitive/StorageLocation.h:129
         llvm::dbgs() << "Existing children:\n";
-        for ([[maybe_unused]] auto [Field, Loc] : Children) {
+        for ([[maybe_unused]] const auto &[Field, Loc] : Children) {
           llvm::dbgs() << Field->getNameAsString() << "\n";
----------------
This is only 2 pointers. I'd rather take them by value.


================
Comment at: clang/lib/AST/Interp/ByteCodeEmitter.cpp:64
 
-      for (auto Cap : LC) {
+      for (const auto &Cap : LC) {
         unsigned Offset = R->getField(Cap.second)->Offset;
----------------
Only 2 pointers.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3728
     const CXXRecordDecl *Base = nullptr;
-    for (auto I : CXXRD->bases()) {
+    for (const auto &I : CXXRD->bases()) {
       if (I.isVirtual())
----------------
It's 24 bytes, which is like 3 pointers.


================
Comment at: clang/lib/Analysis/FlowSensitive/RecordOps.cpp:38
 
-  for (auto [Field, DstFieldLoc] : Dst.children()) {
+  for (const auto &[Field, DstFieldLoc] : Dst.children()) {
     StorageLocation *SrcFieldLoc = Src.getChild(*Field);
----------------
2 pointers.


================
Comment at: clang/lib/Analysis/FlowSensitive/RecordOps.cpp:86
 
-  for (auto [Field, FieldLoc1] : Loc1.children()) {
+  for (const auto &[Field, FieldLoc1] : Loc1.children()) {
     StorageLocation *FieldLoc2 = Loc2.getChild(*Field);
----------------
2 pointers.


================
Comment at: clang/lib/Sema/SemaRISCVVectorLookup.cpp:305
         // Create masked policy intrinsic.
-        for (auto P : SupportedMaskedPolicies) {
+        for (const auto &P : SupportedMaskedPolicies) {
           llvm::SmallVector<PrototypeDescriptor> PolicyPrototype =
----------------
It's just a single `long`. 8 bytes.
Taking by ref would definitely not help the situation.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2621
   CStringLengthTy::Factory &F = state->get_context<CStringLength>();
-  for (auto [Reg, Len] : Entries) {
+  for (auto &[Reg, Len] : Entries) {
     if (SymbolRef Sym = Len.getAsSymbol()) {
----------------
8 + 16 bytes. Basically 3 pointers.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:851-852
       if (const ObjCMethodDecl *OMD = dyn_cast_or_null<ObjCMethodDecl>(D)) {
-        for (auto [Idx, FormalParam] : llvm::enumerate(OMD->parameters())) {
+        for (const auto &[Idx, FormalParam] :
+             llvm::enumerate(OMD->parameters())) {
           if (isAnnotatedAsTakingLocalized(FormalParam)) {
----------------
size_t and a pointer. 16 bytes in total.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp:534
   if (ReturnSymbol)
-    for (auto [Sym, AllocState] : AMap) {
+    for (auto &[Sym, AllocState] : AMap) {
       if (ReturnSymbol == AllocState.Region)
----------------
8 + 16 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2821
   ReallocPairsTy RP = state->get<ReallocPairs>();
-  for (auto [Sym, ReallocPair] : RP) {
+  for (auto &[Sym, ReallocPair] : RP) {
     if (SymReaper.isDead(Sym) || SymReaper.isDead(ReallocPair.ReallocatedSym)) {
----------------
8 + 16 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3081
   ReallocPairsTy RP = state->get<ReallocPairs>();
-  for (auto [Sym, ReallocPair] : RP) {
+  for (auto &[Sym, ReallocPair] : RP) {
     // If the symbol is assumed to be NULL, remove it from consideration.
----------------
8 + 16 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3558
     Out << Sep << "MallocChecker :" << NL;
-    for (auto [Sym, Data] : RS) {
+    for (auto &[Sym, Data] : RS) {
       const RefState *RefS = State->get<RegionState>(Sym);
----------------
8 + 16 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:945
   PropertyAccessesMapTy PropertyAccesses = State->get<PropertyAccessesMap>();
-  for (auto [PropKey, PropVal] : PropertyAccesses) {
+  for (auto &[PropKey, PropVal] : PropertyAccesses) {
     if (!PropVal.isConstrainedNonnull) {
----------------
16 + 24 bytes. Okay, this is basically the first case where it actually makes sense to take by ref.
But we should still prefer `const ref` over `ref`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:1373
 
-  for (auto [Region, State] : B) {
+  for (auto &[Region, State] : B) {
     Out << Region << " : ";
----------------
8 + 16 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp:365
   Out << NL;
-  for (auto [Sym, Flag] : FlagMap) {
+  for (auto &[Sym, Flag] : FlagMap) {
     Out << Sym << " : ";
----------------
8 + 4 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp:154
   // Find ivars that are unused.
-  for (auto [Ivar, State] : M)
+  for (auto &[Ivar, State] : M)
     if (State == Unused) {
----------------
8 + 4 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp:169
 
-  for (auto Var : ReferencedVars) {
+  for (auto &Var : ReferencedVars) {
     const VarRegion *VR = Var.getCapturedRegion();
----------------
16 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp:166
   StreamMapTy TrackedStreams = State->get<StreamMap>();
-  for (auto [Sym, StreamStatus] : TrackedStreams) {
+  for (auto &[Sym, StreamStatus] : TrackedStreams) {
     bool IsSymDead = SymReaper.isDead(Sym);
----------------
8 + 4 bytes.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1196-1198
+  for (auto const &[Idx, FieldForCapture, InitExpr] :
        llvm::zip(llvm::seq<unsigned>(0, -1), LE->getLambdaClass()->fields(),
                  LE->capture_inits())) {
----------------
Here we are talking about some generator of some pointers and indexes.
I don't think we would benefit from taking this by ref - even though in total it's around 48 bytes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157129