[clang-tools-extra] r246989 - Avoid using rvalue references with trivially copyable types.
Angel Garcia Gomez via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 8 02:01:23 PDT 2015
Author: angelgarcia
Date: Tue Sep 8 04:01:21 2015
New Revision: 246989
URL: http://llvm.org/viewvc/llvm-project?rev=246989&view=rev
Log:
Avoid using rvalue references with trivially copyable types.
Summary:
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.
Reviewers: alexfh, klimek
Subscribers: alexfh, cfe-commits
Differential Revision: http://reviews.llvm.org/D12675
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h
clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp
Modified: clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp?rev=246989&r1=246988&r2=246989&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp Tue Sep 8 04:01:21 2015
@@ -375,7 +375,7 @@ static bool usagesAreConst(const UsageRe
/// by reference.
static bool usagesReturnRValues(const UsageResult &Usages) {
for (const auto &U : Usages) {
- if (!U.Expression->isRValue())
+ if (U.Expression && !U.Expression->isRValue())
return false;
}
return true;
@@ -400,8 +400,7 @@ void LoopConvertCheck::doConversion(
ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer,
StringRef ContainerString, const UsageResult &Usages,
const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit,
- const ForStmt *TheLoop, bool ContainerNeedsDereference, bool DerefByValue,
- bool DerefByConstRef) {
+ const ForStmt *TheLoop, RangeDescriptor Descriptor) {
auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead");
std::string VarName;
@@ -457,16 +456,17 @@ void LoopConvertCheck::doConversion(
// If an iterator's operator*() returns a 'T&' we can bind that to 'auto&'.
// If operator*() returns 'T' we can bind that to 'auto&&' which will deduce
// to 'T&&&'.
- if (DerefByValue) {
- AutoRefType = Context->getRValueReferenceType(AutoRefType);
+ if (Descriptor.DerefByValue) {
+ if (!Descriptor.IsTriviallyCopyable)
+ AutoRefType = Context->getRValueReferenceType(AutoRefType);
} else {
- if (DerefByConstRef)
+ if (Descriptor.DerefByConstRef)
AutoRefType = Context->getConstType(AutoRefType);
AutoRefType = Context->getLValueReferenceType(AutoRefType);
}
}
- StringRef MaybeDereference = ContainerNeedsDereference ? "*" : "";
+ StringRef MaybeDereference = Descriptor.ContainerNeedsDereference ? "*" : "";
std::string TypeString = AutoRefType.getAsString();
std::string Range = ("(" + TypeString + " " + VarName + " : " +
MaybeDereference + ContainerString + ")")
@@ -518,11 +518,11 @@ StringRef LoopConvertCheck::checkRejecti
/// of the index variable and convert the loop if possible.
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) {
+ const Expr *ContainerExpr, const Expr *BoundExpr, const ForStmt *TheLoop,
+ LoopFixerKind FixerKind, RangeDescriptor Descriptor) {
ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
- BoundExpr, ContainerNeedsDereference);
+ BoundExpr,
+ Descriptor.ContainerNeedsDereference);
if (ContainerExpr) {
ComponentFinderASTVisitor ComponentFinder;
@@ -544,15 +544,28 @@ void LoopConvertCheck::findAndVerifyUsag
!isDirectMemberExpr(ContainerExpr))
ConfidenceLevel.lowerTo(Confidence::CL_Risky);
} else if (FixerKind == LFK_PseudoArray) {
- if (!DerefByValue && !DerefByConstRef) {
+ if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) {
const UsageResult &Usages = Finder.getUsages();
if (usagesAreConst(Usages)) {
- // FIXME: check if the type is trivially copiable.
- DerefByConstRef = true;
+ Descriptor.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;
+ Descriptor.DerefByValue = true;
+ // Try to find the type of the elements on the container from the
+ // usages.
+ for (const Usage &U : Usages) {
+ if (!U.Expression || U.Expression->getType().isNull())
+ continue;
+ QualType Type = U.Expression->getType().getCanonicalType();
+ if (U.IsArrow) {
+ if (!Type->isPointerType())
+ continue;
+ Type = Type->getPointeeType();
+ }
+ Descriptor.IsTriviallyCopyable =
+ Type.isTriviallyCopyableType(*Context);
+ }
}
}
}
@@ -565,7 +578,7 @@ void LoopConvertCheck::findAndVerifyUsag
doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
ContainerString, Finder.getUsages(), Finder.getAliasDecl(),
Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop,
- ContainerNeedsDereference, DerefByValue, DerefByConstRef);
+ Descriptor);
}
void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
@@ -618,15 +631,13 @@ void LoopConvertCheck::check(const Match
ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
const Expr *ContainerExpr = nullptr;
- bool DerefByValue = false;
- bool DerefByConstRef = false;
- bool ContainerNeedsDereference = false;
+ RangeDescriptor Descriptor{false, false, false, false};
// FIXME: Try to put most of this logic inside a matcher. Currently, matchers
// don't allow the ight-recursive checks in digThroughConstructors.
if (FixerKind == LFK_Iterator) {
ContainerExpr = findContainer(Context, LoopVar->getInit(),
EndVar ? EndVar->getInit() : EndCall,
- &ContainerNeedsDereference);
+ &Descriptor.ContainerNeedsDereference);
QualType InitVarType = InitVar->getType();
QualType CanonicalInitVarType = InitVarType.getCanonicalType();
@@ -643,6 +654,8 @@ void LoopConvertCheck::check(const Match
// un-qualified pointee types match otherwise we don't use auto.
if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType))
return;
+ Descriptor.IsTriviallyCopyable =
+ BeginPointeeType.isTriviallyCopyableType(*Context);
} else {
// Check for qualified types to avoid conversions from non-const to const
// iterator types.
@@ -650,17 +663,19 @@ void LoopConvertCheck::check(const Match
return;
}
- DerefByValue = Nodes.getNodeAs<QualType>(DerefByValueResultName) != nullptr;
- if (!DerefByValue) {
+ const auto *DerefByValueType =
+ Nodes.getNodeAs<QualType>(DerefByValueResultName);
+ Descriptor.DerefByValue = DerefByValueType;
+ if (!Descriptor.DerefByValue) {
if (const auto *DerefType =
Nodes.getNodeAs<QualType>(DerefByRefResultName)) {
// A node will only be bound with DerefByRefResultName if we're dealing
// with a user-defined iterator type. Test the const qualification of
// the reference type.
- DerefByConstRef = (*DerefType)
- ->getAs<ReferenceType>()
- ->getPointeeType()
- .isConstQualified();
+ Descriptor.DerefByConstRef = (*DerefType)
+ ->getAs<ReferenceType>()
+ ->getPointeeType()
+ .isConstQualified();
} else {
// By nature of the matcher this case is triggered only for built-in
// iterator types (i.e. pointers).
@@ -671,12 +686,14 @@ void LoopConvertCheck::check(const Match
// If the initializer and variable have both the same type just use auto
// otherwise we test for const qualification of the pointed-at type.
if (!Context->hasSameType(InitPointeeType, BeginPointeeType))
- DerefByConstRef = InitPointeeType.isConstQualified();
+ Descriptor.DerefByConstRef = InitPointeeType.isConstQualified();
}
} else {
// If the dereference operator returns by value then test for the
// canonical const qualification of the init variable type.
- DerefByConstRef = CanonicalInitVarType.isConstQualified();
+ Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified();
+ Descriptor.IsTriviallyCopyable =
+ DerefByValueType->isTriviallyCopyableType(*Context);
}
} else if (FixerKind == LFK_PseudoArray) {
if (!EndCall)
@@ -685,7 +702,7 @@ void LoopConvertCheck::check(const Match
const auto *Member = dyn_cast<MemberExpr>(EndCall->getCallee());
if (!Member)
return;
- ContainerNeedsDereference = Member->isArrow();
+ Descriptor.ContainerNeedsDereference = Member->isArrow();
}
// We must know the container or an array length bound.
@@ -696,8 +713,7 @@ void LoopConvertCheck::check(const Match
return;
findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr,
- ContainerNeedsDereference, DerefByValue, DerefByConstRef,
- TheLoop, FixerKind);
+ TheLoop, FixerKind, Descriptor);
}
} // namespace modernize
Modified: clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h?rev=246989&r1=246988&r2=246989&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h Tue Sep 8 04:01:21 2015
@@ -25,22 +25,27 @@ public:
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
+ struct RangeDescriptor {
+ bool ContainerNeedsDereference;
+ bool DerefByValue;
+ bool IsTriviallyCopyable;
+ bool DerefByConstRef;
+ };
+
void doConversion(ASTContext *Context, const VarDecl *IndexVar,
const VarDecl *MaybeContainer, StringRef ContainerString,
const UsageResult &Usages, const DeclStmt *AliasDecl,
bool AliasUseRequired, bool AliasFromForInit,
- const ForStmt *TheLoop, bool ContainerNeedsDereference,
- bool DerefByValue, bool DerefByConstRef);
+ const ForStmt *TheLoop, RangeDescriptor Descriptor);
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);
+ const Expr *BoundExpr, const ForStmt *TheLoop,
+ LoopFixerKind FixerKind, RangeDescriptor Descriptor);
+
std::unique_ptr<TUTrackingInfo> TUInfo;
Confidence::Level MinConfidence;
};
Modified: clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp?rev=246989&r1=246988&r2=246989&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp Tue Sep 8 04:01:21 2015
@@ -349,11 +349,13 @@ static bool isAliasDecl(ASTContext *Cont
// Check that the declared type is the same as (or a reference to) the
// container type.
+ QualType InitType = Init->getType();
QualType DeclarationType = VDecl->getType();
- if (DeclarationType->isReferenceType())
+ if (!DeclarationType.isNull() && DeclarationType->isReferenceType())
DeclarationType = DeclarationType.getNonReferenceType();
- QualType InitType = Init->getType();
- if (!Context->hasSameUnqualifiedType(DeclarationType, InitType))
+
+ if (InitType.isNull() || DeclarationType.isNull() ||
+ !Context->hasSameUnqualifiedType(DeclarationType, InitType))
return false;
switch (Init->getStmtClass()) {
Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp?rev=246989&r1=246988&r2=246989&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp Tue Sep 8 04:01:21 2015
@@ -291,7 +291,7 @@ void f() {
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,14 +564,23 @@ struct DerefByValue {
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) {
+ // CHECK-FIXES-NEXT: printf("%d\n", elem);
+ for (unsigned i = 0, e = DBV.size(); i < e; ++i) {
+ auto f = [DBV, i]() {};
+ printf("%d\n", DBV[i]);
+ }
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto elem : DBV) {
+ // CHECK-FIXES-NEXT: auto f = [DBV, elem]() {};
+ // CHECK-FIXES-NEXT: printf("%d\n", elem);
}
} // namespace PseudoArray
More information about the cfe-commits
mailing list