[PATCH] D148812: [NFC][clang] Fix static analyzer concerns

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:8206
 
-  for (auto B : CXXRD->bases())
+  for (const auto &B : CXXRD->bases())
     if (hasTemplateSpecializationInEncodedString(B.getType().getTypePtr(),
----------------
Manna wrote:
> CXXBaseSpecifier is less in size, but most of cases we are using reference semantics.
A bit bigger than 2 pointers actually, its ~1 byte in bitfields, 12 bytes in `SourceLocation`, and a pointer.

So perhaps still on the line.  However, it is only dereferenced 1x.


================
Comment at: clang/lib/Lex/Pragma.cpp:1110
       Module *M = nullptr;
-      for (auto IIAndLoc : ModuleName) {
+      for (const auto &IIAndLoc : ModuleName) {
         M = MM.lookupModuleQualified(IIAndLoc.first->getName(), M);
----------------
Manna wrote:
> This returns a std::pair<IdentifierInfo *, SourceLocation> which is not particularly expensive to copy
In that case, I'd skip the change here and leave it as a copy.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4262
   // will be using.
-  for (auto I : Attrs) {
+  for (const auto &I : Attrs) {
     const Record &Attr = *I.second;
----------------
Manna wrote:
> Passes as a reference 
Each element is a pair of a `std::string` and `const Record*`, which should justify this.  Not sure what you mean by 'passes as a reference'?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:562
         if (UnMaskedPolicyScheme != PolicyScheme::SchemeNone)
-          for (auto P : SupportedUnMaskedPolicies) {
+          for (const auto &P : SupportedUnMaskedPolicies) {
             SmallVector<PrototypeDescriptor> PolicyPrototype =
----------------
What type is 'P' here?  What size is it?  It is being used quite a few times in this loop, which means it would have to be pretty sizable to justify making it a reference unless it has a copy ctor of cost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148812