angelgarcia created this revision.
angelgarcia added a reviewer: klimek.
angelgarcia added subscribers: cfe-commits, alexfh.
When the dereference operator returns a value that is trivially
copyable (like a pointer), copy it. After this change, modernize-loop-convert
check can be applied to the whole llvm source code without breaking any build
or test.
http://reviews.llvm.org/D12675
Files:
clang-tidy/modernize/LoopConvertCheck.cpp
clang-tidy/modernize/LoopConvertCheck.h
test/clang-tidy/modernize-loop-convert-basic.cpp
Index: test/clang-tidy/modernize-loop-convert-basic.cpp
===================================================================
--- test/clang-tidy/modernize-loop-convert-basic.cpp
+++ test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -291,7 +291,7 @@
I != E; ++I) {
}
// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead
- // CHECK-FIXES: for (auto && int_ptr : int_ptrs) {
+ // CHECK-FIXES: for (auto int_ptr : int_ptrs) {
}
// This container uses an iterator where the derefence type is a typedef of
@@ -564,13 +564,13 @@
unsigned operator[](int);
};
-void DerefByValueTest() {
+void derefByValueTest() {
DerefByValue DBV;
for (unsigned i = 0, e = DBV.size(); i < e; ++i) {
printf("%d\n", DBV[i]);
}
// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
- // CHECK-FIXES: for (auto && elem : DBV) {
+ // CHECK-FIXES: for (auto elem : DBV) {
}
Index: clang-tidy/modernize/LoopConvertCheck.h
===================================================================
--- clang-tidy/modernize/LoopConvertCheck.h
+++ clang-tidy/modernize/LoopConvertCheck.h
@@ -30,17 +30,18 @@
const UsageResult &Usages, const DeclStmt *AliasDecl,
bool AliasUseRequired, bool AliasFromForInit,
const ForStmt *TheLoop, bool ContainerNeedsDereference,
- bool DerefByValue, bool DerefByConstRef);
+ bool DerefByValue, bool IsTriviallyCopyable,
+ bool DerefByConstRef);
StringRef checkRejections(ASTContext *Context, const Expr *ContainerExpr,
const ForStmt *TheLoop);
void findAndVerifyUsages(ASTContext *Context, const VarDecl *LoopVar,
const VarDecl *EndVar, const Expr *ContainerExpr,
const Expr *BoundExpr,
bool ContainerNeedsDereference, bool DerefByValue,
- bool DerefByConstRef, const ForStmt *TheLoop,
- LoopFixerKind FixerKind);
+ bool IsTriviallyCopyable, bool DerefByConstRef,
+ const ForStmt *TheLoop, LoopFixerKind FixerKind);
std::unique_ptr<TUTrackingInfo> TUInfo;
Confidence::Level MinConfidence;
};
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -401,7 +401,7 @@
StringRef ContainerString, const UsageResult &Usages,
const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit,
const ForStmt *TheLoop, bool ContainerNeedsDereference, bool DerefByValue,
- bool DerefByConstRef) {
+ bool IsTriviallyCopyable, bool DerefByConstRef) {
auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead");
std::string VarName;
@@ -458,7 +458,8 @@
// If operator*() returns 'T' we can bind that to 'auto&&' which will deduce
// to 'T&&&'.
if (DerefByValue) {
- AutoRefType = Context->getRValueReferenceType(AutoRefType);
+ if (!IsTriviallyCopyable)
+ AutoRefType = Context->getRValueReferenceType(AutoRefType);
} else {
if (DerefByConstRef)
AutoRefType = Context->getConstType(AutoRefType);
@@ -519,8 +520,8 @@
void LoopConvertCheck::findAndVerifyUsages(
ASTContext *Context, const VarDecl *LoopVar, const VarDecl *EndVar,
const Expr *ContainerExpr, const Expr *BoundExpr,
- bool ContainerNeedsDereference, bool DerefByValue, bool DerefByConstRef,
- const ForStmt *TheLoop, LoopFixerKind FixerKind) {
+ bool ContainerNeedsDereference, bool DerefByValue, bool IsTriviallyCopyable,
+ bool DerefByConstRef, const ForStmt *TheLoop, LoopFixerKind FixerKind) {
ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
BoundExpr, ContainerNeedsDereference);
@@ -547,12 +548,20 @@
if (!DerefByValue && !DerefByConstRef) {
const UsageResult &Usages = Finder.getUsages();
if (usagesAreConst(Usages)) {
- // FIXME: check if the type is trivially copiable.
DerefByConstRef = true;
} else if (usagesReturnRValues(Usages)) {
// If the index usages (dereference, subscript, at) return RValues,
// then we should not use a non-const reference.
DerefByValue = true;
+ // We can assume that we have at least one Usage, because
+ // usagesAreConst() returned false.
+ QualType Type = Usages[0].Expression->getType().getCanonicalType();
+ if (Usages[0].IsArrow) {
+ if (!Type->isPointerType())
+ return;
+ Type = Type->getPointeeType();
+ }
+ IsTriviallyCopyable = Type.isTriviallyCopyableType(*Context);
}
}
}
@@ -565,7 +574,8 @@
doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
ContainerString, Finder.getUsages(), Finder.getAliasDecl(),
Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop,
- ContainerNeedsDereference, DerefByValue, DerefByConstRef);
+ ContainerNeedsDereference, DerefByValue, IsTriviallyCopyable,
+ DerefByConstRef);
}
void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
@@ -619,6 +629,7 @@
const Expr *ContainerExpr = nullptr;
bool DerefByValue = false;
+ bool IsTriviallyCopyable = false;
bool DerefByConstRef = false;
bool ContainerNeedsDereference = false;
// FIXME: Try to put most of this logic inside a matcher. Currently, matchers
@@ -643,14 +654,17 @@
// un-qualified pointee types match otherwise we don't use auto.
if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType))
return;
+ IsTriviallyCopyable = BeginPointeeType.isTriviallyCopyableType(*Context);
} else {
// Check for qualified types to avoid conversions from non-const to const
// iterator types.
if (!Context->hasSameType(CanonicalInitVarType, CanonicalBeginType))
return;
}
- DerefByValue = Nodes.getNodeAs<QualType>(DerefByValueResultName) != nullptr;
+ const auto *DerefByValueType =
+ Nodes.getNodeAs<QualType>(DerefByValueResultName);
+ DerefByValue = DerefByValueType != nullptr;
if (!DerefByValue) {
if (const auto *DerefType =
Nodes.getNodeAs<QualType>(DerefByRefResultName)) {
@@ -677,6 +691,7 @@
// If the dereference operator returns by value then test for the
// canonical const qualification of the init variable type.
DerefByConstRef = CanonicalInitVarType.isConstQualified();
+ IsTriviallyCopyable = DerefByValueType->isTriviallyCopyableType(*Context);
}
} else if (FixerKind == LFK_PseudoArray) {
if (!EndCall)
@@ -696,8 +711,8 @@
return;
findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr,
- ContainerNeedsDereference, DerefByValue, DerefByConstRef,
- TheLoop, FixerKind);
+ ContainerNeedsDereference, DerefByValue,
+ IsTriviallyCopyable, DerefByConstRef, TheLoop, FixerKind);
}
} // namespace modernize
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
