Hi tareqsiraj,
If the LoopConvert Transform detects an alias for the loop variable, it
attempts to use that name in the resulting range-based for loop while
removing the original DeclStmt for the variable. That removal produced
bad code when the declaration was in the condition of an if, switch,
while, or for stmt. This revision fixes the problem by simply replacing
the declaration with a use of the alias variable.
Aliases declared in the init clause of a nested for loop are still
removed.
http://llvm-reviews.chandlerc.com/D752
Files:
cpp11-migrate/LoopConvert/LoopActions.cpp
cpp11-migrate/LoopConvert/LoopActions.h
test/cpp11-migrate/LoopConvert/naming-alias.cpp
Index: cpp11-migrate/LoopConvert/LoopActions.cpp
===================================================================
--- cpp11-migrate/LoopConvert/LoopActions.cpp
+++ cpp11-migrate/LoopConvert/LoopActions.cpp
@@ -72,7 +72,9 @@
Context(Context), IndexVar(IndexVar), EndVar(EndVar),
ContainerExpr(ContainerExpr), ArrayBoundExpr(ArrayBoundExpr),
ContainerNeedsDereference(ContainerNeedsDereference),
- OnlyUsedAsIndex(true), AliasDecl(NULL), ConfidenceLevel(RL_Safe) {
+ OnlyUsedAsIndex(true), AliasDecl(NULL), ConfidenceLevel(RL_Safe),
+ NextStmtParent(NULL), CurrStmtParent(NULL), ReplaceWithAliasUse(false),
+ AliasFromForInit(false) {
if (ContainerExpr) {
addComponent(ContainerExpr);
llvm::FoldingSetNodeID ID;
@@ -123,6 +125,17 @@
return ConfidenceLevel.getRiskLevel();
}
+ /// \brief Indicates if the alias declaration was in a place where it cannot
+ /// simply be removed but rather replaced with a use of the alias variable.
+ /// For example, variables declared in the condition of an if, switch, or for
+ /// stmt.
+ bool aliasUseRequired() const { return ReplaceWithAliasUse; }
+
+ /// \brief Indicates if the alias declaration came from the init clause of a
+ /// nested for loop. SourceRanges provided by Clang for DeclStmts in this
+ /// case need to be adjusted.
+ bool aliasFromForInit() const { return AliasFromForInit; }
+
private:
/// Typedef used in CRTP functions.
typedef RecursiveASTVisitor<ForLoopIndexUseVisitor> VisitorBase;
@@ -136,6 +149,7 @@
bool TraverseUnaryDeref(UnaryOperator *Uop);
bool VisitDeclRefExpr(DeclRefExpr *E);
bool VisitDeclStmt(DeclStmt *S);
+ bool TraverseStmt(Stmt *S);
/// \brief Add an expression to the list of expressions on which the container
/// expression depends.
@@ -172,6 +186,11 @@
/// of the loop element, lower our confidence level.
llvm::SmallVector<
std::pair<const Expr *, llvm::FoldingSetNodeID>, 16> DependentExprs;
+
+ const Stmt *NextStmtParent;
+ const Stmt *CurrStmtParent;
+ bool ReplaceWithAliasUse;
+ bool AliasFromForInit;
};
/// \brief Obtain the original source code text from a SourceRange.
@@ -722,18 +741,45 @@
/// See the comments for isAliasDecl.
bool ForLoopIndexUseVisitor::VisitDeclStmt(DeclStmt *S) {
if (!AliasDecl && S->isSingleDecl() &&
- isAliasDecl(S->getSingleDecl(), IndexVar))
- AliasDecl = S;
+ isAliasDecl(S->getSingleDecl(), IndexVar)) {
+ AliasDecl = S;
+ if (CurrStmtParent) {
+ if (isa<IfStmt>(CurrStmtParent) ||
+ isa<WhileStmt>(CurrStmtParent) ||
+ isa<SwitchStmt>(CurrStmtParent))
+ ReplaceWithAliasUse = true;
+ else if (isa<ForStmt>(CurrStmtParent)) {
+ if (cast<ForStmt>(CurrStmtParent)->getConditionVariableDeclStmt() == S)
+ ReplaceWithAliasUse = true;
+ else
+ // It's assumed S came the for loop's init clause.
+ AliasFromForInit = true;
+ }
+ }
+ }
+
return true;
}
+bool ForLoopIndexUseVisitor::TraverseStmt(Stmt *S) {
+ const Stmt *OldNextParent = NextStmtParent;
+ CurrStmtParent = NextStmtParent;
+ NextStmtParent = S;
+ bool Result = VisitorBase::TraverseStmt(S);
+ NextStmtParent = OldNextParent;
+ return Result;
+}
+
//// \brief Apply the source transformations necessary to migrate the loop!
void LoopFixer::doConversion(ASTContext *Context,
const VarDecl *IndexVar,
const VarDecl *MaybeContainer,
StringRef ContainerString,
const UsageResult &Usages,
- const DeclStmt *AliasDecl, const ForStmt *TheLoop,
+ const DeclStmt *AliasDecl,
+ bool AliasUseRequired,
+ bool AliasFromForInit,
+ const ForStmt *TheLoop,
bool ContainerNeedsDereference,
bool DerefByValue) {
std::string VarName;
@@ -747,9 +793,19 @@
// We keep along the entire DeclStmt to keep the correct range here.
const SourceRange &ReplaceRange = AliasDecl->getSourceRange();
- Replace->insert(
- Replacement(Context->getSourceManager(),
- CharSourceRange::getTokenRange(ReplaceRange), ""));
+
+ std::string ReplacementText;
+ if (AliasUseRequired)
+ ReplacementText = VarName;
+ else if (AliasFromForInit)
+ // FIXME: Clang includes the location of the ';' but only for DeclStmt's
+ // in a for loop's init clause. Need to put this ';' back while removing
+ // the declaration of the alias variable. This is probably a bug.
+ ReplacementText = ";";
+
+ Replace->insert(Replacement(Context->getSourceManager(),
+ CharSourceRange::getTokenRange(ReplaceRange),
+ ReplacementText));
// No further replacements are made to the loop, since the iterator or index
// was used exactly once - in the initialization of AliasVar.
} else {
@@ -945,9 +1001,9 @@
return;
doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),
- ContainerString, Finder.getUsages(),
- Finder.getAliasDecl(), TheLoop, ContainerNeedsDereference,
- DerefByValue);
+ ContainerString, Finder.getUsages(), Finder.getAliasDecl(),
+ Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop,
+ ContainerNeedsDereference, DerefByValue);
++*AcceptedChanges;
}
Index: cpp11-migrate/LoopConvert/LoopActions.h
===================================================================
--- cpp11-migrate/LoopConvert/LoopActions.h
+++ cpp11-migrate/LoopConvert/LoopActions.h
@@ -73,6 +73,8 @@
llvm::StringRef ContainerString,
const UsageResult &Usages,
const clang::DeclStmt *AliasDecl,
+ bool AliasUseRequired,
+ bool AliasFromForInit,
const clang::ForStmt *TheLoop,
bool ContainerNeedsDereference,
bool DerefByValue);
Index: test/cpp11-migrate/LoopConvert/naming-alias.cpp
===================================================================
--- test/cpp11-migrate/LoopConvert/naming-alias.cpp
+++ test/cpp11-migrate/LoopConvert/naming-alias.cpp
@@ -8,6 +8,7 @@
Val Arr[N];
Val &func(Val &);
+void sideEffect(int);
void aliasing() {
// If the loop container is only used for a declaration of a temporary
@@ -56,6 +57,48 @@
// CHECK: for (auto & elem : Arr)
// CHECK-NEXT: Val &t = func(elem);
// CHECK-NEXT: int y = t.x;
+
+ int IntArr[N];
+ for (unsigned i = 0; i < N; ++i) {
+ if (int alias = IntArr[i]) {
+ sideEffect(alias);
+ }
+ }
+ // CHECK: for (auto alias : IntArr)
+ // CHECK-NEXT: if (alias) {
+
+ for (unsigned i = 0; i < N; ++i) {
+ while (int alias = IntArr[i]) {
+ sideEffect(alias);
+ }
+ }
+ // CHECK: for (auto alias : IntArr)
+ // CHECK-NEXT: while (alias) {
+
+ for (unsigned i = 0; i < N; ++i) {
+ switch (int alias = IntArr[i]) {
+ default:
+ sideEffect(alias);
+ }
+ }
+ // CHECK: for (auto alias : IntArr)
+ // CHECK-NEXT: switch (alias) {
+
+ for (unsigned i = 0; i < N; ++i) {
+ for (int alias = IntArr[i]; alias < N; ++alias) {
+ sideEffect(alias);
+ }
+ }
+ // CHECK: for (auto alias : IntArr)
+ // CHECK-NEXT: for (; alias < N; ++alias) {
+
+ for (unsigned i = 0; i < N; ++i) {
+ for (unsigned j = 0; int alias = IntArr[i]; ++j) {
+ sideEffect(alias);
+ }
+ }
+ // CHECK: for (auto alias : IntArr)
+ // CHECK-NEXT: for (unsigned j = 0; alias; ++j) {
}
void refs_and_vals() {
@@ -93,4 +136,5 @@
// CHECK-NOT: MutableVal &{{[a-z_]+}} =
// CHECK-NEXT: { }
// CHECK-NEXT: alias.x = 0;
+
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
