aaron.ballman added a comment.
Thanks for the static analysis fixes, however, it looks like quite a few of
them were false positives again. When fixing any static analysis diagnostics,
please put in the effort to determine whether it's actually correct or not, as
it seem the Coverity diagnostic is not really paying any attention to the size
of what's being copied. If you're applying the advice from the tool without
further analysis, it can be disruptive (might introduce bugs, churns the code
for negative/no benefit, lowers reviewer confidence in the rest of the changes,
etc). (Not suggesting you're blindly applying advice, but mostly observing
we've had a few reviews like this.)
My suggestion is to always look at the size of what's being copied. As a
general rule of thumb, if it's two pointers or less in size, the copy is likely
cheaper than the reference and shouldn't be changed.
================
Comment at: clang/lib/AST/ExprConstant.cpp:6117
} else
- for (auto Idx : Attr->args()) {
+ for (const auto &Idx : Attr->args()) {
unsigned ASTIdx = Idx.getASTIndex();
----------------
This returns a `ParamIdx` object which is 32-bits, so it's not expensive to
copy.
================
Comment at: clang/lib/Basic/TargetID.cpp:136
OrderedMap[F.first()] = F.second;
- for (auto F : OrderedMap)
+ for (const auto &F : OrderedMap)
TargetID = TargetID + ':' + F.first.str() + (F.second ? "+" : "-");
----------------
The map iterator returns a `std::pair<StringRef, bool>` which is not
particularly expensive to copy, but I think this is borderline enough that it's
fine to keep.
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:463
- for (auto Feature : TargetFeatures)
+ for (const auto &Feature : TargetFeatures)
if (Feature[0] == '+')
----------------
This returns a `StringRef` which is not expensive to copy.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3548
SmallVector<std::pair<llvm::Value *, SanitizerMask>, 5> Checks;
- for (auto CheckKindMaskPair : CheckKinds) {
+ for (const auto &CheckKindMaskPair : CheckKinds) {
int Kind = CheckKindMaskPair.first;
----------------
This is a pair of ints, not expensive to copy.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:9426
} else {
for (auto IvarPair : DuplicateIvars) {
Diag(IvarPair.first->getLocation(),
----------------
Why is the above considered too expensive but this one is not?
================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:989
std::vector<FileID> Keys;
- for (auto F : Files)
+ for (const auto &F : Files)
Keys.push_back(F.first);
----------------
`FileID` is an `int`, not expensive to copy.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148639/new/
https://reviews.llvm.org/D148639
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
