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

[Srividya Sundaram via Phabricator via cfe-commits]

srividya-sundaram abandoned this revision.
srividya-sundaram added a comment.

I did spend a good amount of time trying to figure out how to abandon this 
review and gave up eventually. 
Even after your comment, I had to search a bit for the "Add Action.." drop 
down. Clearly, I'm UI challenged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157129

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Srividya Sundaram via Phabricator via cfe-commits]

srividya-sundaram added a comment.

In D157129#4564766 , @steakhal wrote:

> 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.

Thank you, @steakhal for the detailed feedback.
Based on your comments, and after discussing with @tahonermann, I am abandoning 
this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157129

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Srividya Sundaram via Phabricator via cfe-commits]

srividya-sundaram updated this revision to Diff 547350.
srividya-sundaram added a comment.

Added 'const' type qualifiers per review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157129

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/lib/AST/Interp/ByteCodeEmitter.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Analysis/FlowSensitive/RecordOps.cpp
  clang/lib/Sema/SemaRISCVVectorLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/ProgramState.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1089,7 +1089,7 @@
   // BlockDataRegion?  If so, invalidate captured variables that are passed
   // by reference.
   if (const BlockDataRegion *BR = dyn_cast(baseR)) {
-for (auto Var : BR->referenced_vars()) {
+for (auto &Var : BR->referenced_vars()) {
   const VarRegion *VR = Var.getCapturedRegion();
   const VarDecl *VD = VR->getDecl();
   if (VD->hasAttr() || !VD->hasLocalStorage()) {
@@ -2844,7 +2844,7 @@
 
 // All regions captured by a block are also live.
 if (const BlockDataRegion *BR = dyn_cast(R)) {
-  for (auto Var : BR->referenced_vars())
+  for (auto &Var : BR->referenced_vars())
 AddToWorkList(Var.getCapturedRegion());
 }
   }
Index: clang/lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- clang/lib/StaticAnalyzer/Core/ProgramState.cpp
+++ clang/lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -627,7 +627,7 @@
 
   // Regions captured by a block are also implicitly reachable.
   if (const BlockDataRegion *BDR = dyn_cast(R)) {
-for (auto Var : BDR->referenced_vars()) {
+for (auto &Var : BDR->referenced_vars()) {
   if (!scan(Var.getCapturedRegion()))
 return false;
 }
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -492,7 +492,7 @@
 void BlockDataRegion::dumpToStream(raw_ostream &os) const {
   os << "block_data{" << BC;
   os << "; ";
-  for (auto Var : referenced_vars())
+  for (auto &Var : referenced_vars())
 os << "(" << Var.getCapturedRegion() << "<-" << Var.getOriginalRegion()
<< ") ";
   os << '}';
@@ -966,7 +966,7 @@
 if (const auto *BC = dyn_cast(LC)) {
   const auto *BR = static_cast(BC->getData());
   // FIXME: This can be made more efficient.
-  for (auto Var : BR->referenced_vars()) {
+  for (auto &Var : BR->referenced_vars()) {
 const TypedValueRegion *OrigR = Var.getOriginalRegion();
 if (const auto *VR = dyn_cast(OrigR)) {
   if (VR->getDecl() == VD)
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1193,7 +1193,7 @@
 
   // If we created a new MemRegion for the lambda, we should explicitly bind
   // the captures.
-  for (auto const [Idx, FieldForCapture, InitExpr] :
+  for (auto const &[Idx, FieldForCapture, InitExpr] :
llvm::zip(llvm::seq(0, -1), LE->getLambdaClass()->fields(),
  LE->capture_inits())) {
 SVal FieldLoc = State->getLValue(FieldForCapture, V);
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -211,7 +211,7 @@
 auto ReferencedVars = BDR->referenced_vars();
 auto CI = BD->capture_begin();
 auto CE = BD->capture_end();
-for (auto Var : ReferencedVars) {
+for (const auto &Var : ReferencedVars) {
   cons

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

[Srividya Sundaram via Phabricator via cfe-commits]

srividya-sundaram created this revision.
Herald added subscribers: luke, steakhal, frasercrmck, martong, luismarques, 
apazos, sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, 
MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, 
simoncook, johnrusso, rbar, asb.
Herald added a reviewer: NoQ.
Herald added a project: All.
srividya-sundaram requested review of this revision.
Herald added subscribers: cfe-commits, wangpc, MaskRay.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157129

Files:
  clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
  clang/lib/AST/Interp/ByteCodeEmitter.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Analysis/FlowSensitive/RecordOps.cpp
  clang/lib/Sema/SemaRISCVVectorLookup.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCUnusedIVarsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/ProgramState.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1089,7 +1089,7 @@
   // BlockDataRegion?  If so, invalidate captured variables that are passed
   // by reference.
   if (const BlockDataRegion *BR = dyn_cast(baseR)) {
-for (auto Var : BR->referenced_vars()) {
+for (auto &Var : BR->referenced_vars()) {
   const VarRegion *VR = Var.getCapturedRegion();
   const VarDecl *VD = VR->getDecl();
   if (VD->hasAttr() || !VD->hasLocalStorage()) {
@@ -2844,7 +2844,7 @@
 
 // All regions captured by a block are also live.
 if (const BlockDataRegion *BR = dyn_cast(R)) {
-  for (auto Var : BR->referenced_vars())
+  for (auto &Var : BR->referenced_vars())
 AddToWorkList(Var.getCapturedRegion());
 }
   }
Index: clang/lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- clang/lib/StaticAnalyzer/Core/ProgramState.cpp
+++ clang/lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -627,7 +627,7 @@
 
   // Regions captured by a block are also implicitly reachable.
   if (const BlockDataRegion *BDR = dyn_cast(R)) {
-for (auto Var : BDR->referenced_vars()) {
+for (auto &Var : BDR->referenced_vars()) {
   if (!scan(Var.getCapturedRegion()))
 return false;
 }
Index: clang/lib/StaticAnalyzer/Core/MemRegion.cpp
===
--- clang/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ clang/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -492,7 +492,7 @@
 void BlockDataRegion::dumpToStream(raw_ostream &os) const {
   os << "block_data{" << BC;
   os << "; ";
-  for (auto Var : referenced_vars())
+  for (auto &Var : referenced_vars())
 os << "(" << Var.getCapturedRegion() << "<-" << Var.getOriginalRegion()
<< ") ";
   os << '}';
@@ -966,7 +966,7 @@
 if (const auto *BC = dyn_cast(LC)) {
   const auto *BR = static_cast(BC->getData());
   // FIXME: This can be made more efficient.
-  for (auto Var : BR->referenced_vars()) {
+  for (auto &Var : BR->referenced_vars()) {
 const TypedValueRegion *OrigR = Var.getOriginalRegion();
 if (const auto *VR = dyn_cast(OrigR)) {
   if (VR->getDecl() == VD)
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1193,7 +1193,7 @@
 
   // If we created a new MemRegion for the lambda, we should explicitly bind
   // the captures.
-  for (auto const [Idx, FieldForCapture, InitExpr] :
+  for (auto const &[Idx, FieldForCapture, InitExpr] :
llvm::zip(llvm::seq(0, -1), LE->getLambdaClass()->fields(),
  LE->capture_inits())) {
 SVal FieldLoc = State->getLValue(FieldForCapture, V);
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cp