[clang-tools-extra] r249300 - Document a bug in loop-convert and fix one of its subcases.
Angel Garcia Gomez via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 5 04:15:40 PDT 2015
Author: angelgarcia
Date: Mon Oct 5 06:15:39 2015
New Revision: 249300
URL: http://llvm.org/viewvc/llvm-project?rev=249300&view=rev
Log:
Document a bug in loop-convert and fix one of its subcases.
Summary: Now that we prioritize copying trivial types over using const-references where possible, I found some cases where, after the transformation, the loop was using the address of the local copy instead of the original object.
Reviewers: klimek
Subscribers: alexfh, cfe-commits
Differential Revision: http://reviews.llvm.org/D13431
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.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=249300&r1=249299&r2=249300&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp Mon Oct 5 06:15:39 2015
@@ -477,6 +477,7 @@ void LoopConvertCheck::doConversion(
std::string VarName;
bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
bool AliasVarIsRef = false;
+ bool CanCopy = true;
if (VarNameFromAlias) {
const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
@@ -525,6 +526,16 @@ void LoopConvertCheck::doConversion(
// and parenthesis around a simple DeclRefExpr can always be
// removed.
Range = Paren->getSourceRange();
+ } else if (const auto *UOP = Parents[0].get<UnaryOperator>()) {
+ // If we are taking the address of the loop variable, then we must
+ // not use a copy, as it would mean taking the address of the loop's
+ // local index instead.
+ // FIXME: This won't catch cases where the address is taken outside
+ // of the loop's body (for instance, in a function that got the
+ // loop's index as a const reference parameter), or where we take
+ // the address of a member (like "&Arr[i].A.B.C").
+ if (UOP->getOpcode() == UO_AddrOf)
+ CanCopy = false;
}
}
} else {
@@ -548,8 +559,10 @@ void LoopConvertCheck::doConversion(
// If the new variable name is from the aliased variable, then the reference
// type for the new variable should only be used if the aliased variable was
// declared as a reference.
- bool UseCopy = (VarNameFromAlias && !AliasVarIsRef) ||
- (Descriptor.DerefByConstRef && Descriptor.IsTriviallyCopyable);
+ bool UseCopy =
+ CanCopy &&
+ ((VarNameFromAlias && !AliasVarIsRef) ||
+ (Descriptor.DerefByConstRef && Descriptor.IsTriviallyCopyable));
if (!UseCopy) {
if (Descriptor.DerefByConstRef) {
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=249300&r1=249299&r2=249300&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 Mon Oct 5 06:15:39 2015
@@ -97,7 +97,7 @@ void f() {
// CHECK-FIXES-NEXT: Tea.g();
}
-void constArray() {
+const int *constArray() {
for (int I = 0; I < N; ++I) {
printf("2 * %d = %d\n", ConstArr[I], ConstArr[I] + ConstArr[I]);
}
@@ -112,6 +112,16 @@ void constArray() {
// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
// CHECK-FIXES: for (const auto & Elem : NonCopy)
// CHECK-FIXES-NEXT: printf("2 * %d = %d\n", Elem.X, Elem.X + Elem.X);
+
+ bool Something = false;
+ for (int I = 0; I < N; ++I) {
+ if (Something)
+ return &ConstArr[I];
+ }
+ // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (const auto & Elem : ConstArr)
+ // CHECK-FIXES-NEXT: if (Something)
+ // CHECK-FIXES-NEXT: return &Elem;
}
struct HasArr {
More information about the cfe-commits
mailing list