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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
