Another patch for modernize-loop-convert.
Summary:
1. Avoid converting loops that iterate over the size of a container and don't use its elements, as this would result in an unused-result warning.
2. Never capture the elements by value on lambdas, thus avoiding doing unnecessary copies and errors with non-copyable types.
3. The 'const auto &' instead of 'auto &' substitution on const containers now works on arrays and pseudoarrays as well.
4. The error about multiple replacements in the same macro call is now documented in the tests (not solved though).
5. Due to [1], I had to add a dummy usage of the range element (like "(void) *It;" or similars) on the tests that had empty loops.
6. I removed the braces from the CHECK comments. I think that there is no need for them, and they confuse vim.
Reviewers: klimek
Subscribers: alexfh, cfe-commits
Differential Revision: http://reviews.llvm.org/D12734
llvm-svn: 247399
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 9af7979..73d93e3 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -381,6 +381,20 @@
return true;
}
+/// \brief Returns true if the container is const-qualified.
+static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) {
+ if (const auto *VDec = getReferencedVariable(ContainerExpr)) {
+ QualType CType = VDec->getType();
+ if (Dereference) {
+ if (!CType->isPointerType())
+ return false;
+ CType = CType->getPointeeType();
+ }
+ return CType.isConstQualified();
+ }
+ return false;
+}
+
LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
MinConfidence(StringSwitch<Confidence::Level>(
@@ -401,6 +415,11 @@
StringRef ContainerString, const UsageResult &Usages,
const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit,
const ForStmt *TheLoop, RangeDescriptor Descriptor) {
+ // If there aren't any usages, converting the loop would generate an unused
+ // variable warning.
+ if (Usages.size() == 0)
+ return;
+
auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead");
std::string VarName;
@@ -436,11 +455,23 @@
VarName = Namer.createIndexName();
// First, replace all usages of the array subscript expression with our new
// variable.
- for (const auto &I : Usages) {
- std::string ReplaceText = I.IsArrow ? VarName + "." : VarName;
+ for (const auto &Usage : Usages) {
+ std::string ReplaceText;
+ if (Usage.Expression) {
+ // If this is an access to a member through the arrow operator, after
+ // the replacement it must be accessed through the '.' operator.
+ ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow ? VarName + "."
+ : VarName;
+ } else {
+ // The Usage expression is only null in case of lambda captures (which
+ // are VarDecl). If the index is captured by value, add '&' to capture
+ // by reference instead.
+ ReplaceText =
+ Usage.Kind == Usage::UK_CaptureByCopy ? "&" + VarName : VarName;
+ }
TUInfo->getReplacedVars().insert(std::make_pair(TheLoop, IndexVar));
Diag << FixItHint::CreateReplacement(
- CharSourceRange::getTokenRange(I.Range), ReplaceText);
+ CharSourceRange::getTokenRange(Usage.Range), ReplaceText);
}
}
@@ -537,16 +568,24 @@
if (FixerKind == LFK_Array) {
// The array being indexed by IndexVar was discovered during traversal.
ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts();
+
// Very few loops are over expressions that generate arrays rather than
// array variables. Consider loops over arrays that aren't just represented
// by a variable to be risky conversions.
if (!getReferencedVariable(ContainerExpr) &&
!isDirectMemberExpr(ContainerExpr))
ConfidenceLevel.lowerTo(Confidence::CL_Risky);
+
+ // Use 'const' if the array is const.
+ if (containerIsConst(ContainerExpr, Descriptor.ContainerNeedsDereference))
+ Descriptor.DerefByConstRef = true;
+
} else if (FixerKind == LFK_PseudoArray) {
if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) {
const UsageResult &Usages = Finder.getUsages();
- if (usagesAreConst(Usages)) {
+ if (usagesAreConst(Usages) ||
+ containerIsConst(ContainerExpr,
+ Descriptor.ContainerNeedsDereference)) {
Descriptor.DerefByConstRef = true;
} else if (usagesReturnRValues(Usages)) {
// If the index usages (dereference, subscript, at) return RValues,
@@ -558,7 +597,7 @@
if (!U.Expression || U.Expression->getType().isNull())
continue;
QualType Type = U.Expression->getType().getCanonicalType();
- if (U.IsArrow) {
+ if (U.Kind == Usage::UK_MemberThroughArrow) {
if (!Type->isPointerType())
continue;
Type = Type->getPointeeType();
@@ -625,6 +664,7 @@
// expression the loop variable is being tested against instead.
const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName);
const auto *BoundExpr = Nodes.getStmtAs<Expr>(ConditionBoundName);
+
// If the loop calls end()/size() after each iteration, lower our confidence
// level.
if (FixerKind != LFK_Array && !EndVar)