[PATCH] D149074: [NFC][clang] Fix Coverity bugs with AUTO_CAUSES_COPY

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

I added a comment to skip the one where the element type is a simple pair. The rest look fine to me.



================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:182
     // Skip methods in records.
-    for (auto P : Context.getParents(*Method)) {
+    for (const auto &P : Context.getParents(*Method)) {
       if (P.template get<CXXRecordDecl>())
----------------
I think this is probably a good change. `getParents()` returns a `DynTypedNodeList` (`clang/include/clang/AST/ParentMapContext.h`) that uses `DynTypedNode` as its element type. `DynTypedNode` (`clang/include/clang/AST/ASTTypeTraits.h`) has a `llvm::AlignedCharArrayUnion` data member that looks like it is sufficiently complicated that avoiding copies is probably advantageous.


================
Comment at: clang/lib/AST/ComputeDependence.cpp:666
   auto Deps = E->getInit()->getDependence();
-  for (auto D : E->designators()) {
+  for (const auto &D : E->designators()) {
     auto DesignatorDeps = ExprDependence::None;
----------------
The element type here is `Designator` (`clang/include/clang/AST/Expr.h`). It functions as a discriminated union; its largest union member has three source locations and an `unsigned` value. Copy constructors look to be trivial, but still, this seems like a win.


================
Comment at: clang/lib/CodeGen/CGGPUBuiltin.cpp:192
   SmallVector<llvm::Value *, 8> Args;
-  for (auto A : CallArgs) {
+  for (const auto &A : CallArgs) {
     // We don't know how to emit non-scalar varargs.
----------------
The element type is `CallArg` (`clang/lib/CodeGen/CGCall.h`). This looks like a clear win; `CallArg` is another discriminated union and its `LValue` member holds a number of pointer members.


================
Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:3760
   int Flags = 0;
-  for (auto Class : Classes) {
+  for (const MSRTTIClass &Class : Classes) {
     if (Class.RD->getNumBases() > 1)
----------------
`MSRTTIClass` (`clang/lib/CodeGen/MicrosoftCXXABI.cpp`) holds two pointers and three `uint32_t` values. Seems like a reasonable change to me.


================
Comment at: clang/lib/Format/Format.cpp:2809
                          unsigned End) {
-  for (auto Range : Ranges) {
+  for (const auto &Range : Ranges) {
     if (Range.getOffset() < End &&
----------------
The element type is `Range` (`clang/include/clang/Tooling/Core/Replacement.h`) and it just holds two `unsigned` values. I don't think this change is needed (though I don't think it would hurt anything either).


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:450
 
-  for (auto BaseSpecifier : CXXRecord->bases()) {
+  for (const auto &BaseSpecifier : CXXRecord->bases()) {
     if (!foundStarOperator)
----------------
The element type is `CXXBaseSpecifier` (`clang/include/clang/AST/DeclCXX.h`). This change seems fine to me.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:4662
 
-  for (auto KNPair : KnownNamespaces)
+  for (const auto &KNPair : KnownNamespaces)
     Namespaces.addNameSpecifier(KNPair.first);
----------------
The element type is a pair of `NamespaceDecl *` and `bool`. I would skip this change.


================
Comment at: clang/utils/TableGen/NeonEmitter.cpp:392
 
-    for (auto Type : Types) {
+    for (const auto &Type : Types) {
       // If this builtin takes an immediate argument, we need to #define it rather
----------------
The element type is `Type` (`clang/utils/TableGen/NeonEmitter.cpp`). It holds a `TypeSpec`, an `enum` value, 5 `bool` values, and 3 `unsigned` values. I'm ambivalent.


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

https://reviews.llvm.org/D149074