As per suggestions, the fix for this issue has been completely reimplemented.
The original variable naming logic will still exist, but the checks for
conflicts has been extended to check for language keywords (including aliases),
macros, and types.
New tests have been added. Also renamed naming.cpp to naming-alias.cpp but I
guess git diff treats it as a new file instead of a rename.
Hi klimek,
http://llvm-reviews.chandlerc.com/D484
CHANGE SINCE LAST DIFF
http://llvm-reviews.chandlerc.com/D484?vs=1165&id=1201#toc
Files:
cpp11-migrate/LoopConvert/LoopActions.cpp
cpp11-migrate/LoopConvert/StmtAncestor.cpp
cpp11-migrate/LoopConvert/StmtAncestor.h
cpp11-migrate/LoopConvert/VariableNaming.cpp
cpp11-migrate/LoopConvert/VariableNaming.h
test/cpp11-migrate/LoopConvert/naming-alias.cpp
test/cpp11-migrate/LoopConvert/naming-conflict.cpp
test/cpp11-migrate/LoopConvert/naming.cpp
Index: cpp11-migrate/LoopConvert/LoopActions.cpp
===================================================================
--- cpp11-migrate/LoopConvert/LoopActions.cpp
+++ cpp11-migrate/LoopConvert/LoopActions.cpp
@@ -749,7 +749,7 @@
// was used exactly once - in the initialization of AliasVar.
} else {
VariableNamer Namer(GeneratedDecls, &ParentFinder->getStmtToParentStmtMap(),
- TheLoop, IndexVar, MaybeContainer);
+ TheLoop, IndexVar, MaybeContainer, Context);
VarName = Namer.createIndexName();
// First, replace all usages of the array subscript expression with our new
// variable.
Index: cpp11-migrate/LoopConvert/StmtAncestor.cpp
===================================================================
--- cpp11-migrate/LoopConvert/StmtAncestor.cpp
+++ cpp11-migrate/LoopConvert/StmtAncestor.cpp
@@ -115,3 +115,25 @@
return VisitNamedDecl(D);
return true;
}
+
+/// \brief If the new variable name conflicts with any type used in the loop,
+/// then we mark that variable name as taken.
+bool DeclFinderASTVisitor::VisitTypeLoc(TypeLoc TL) {
+ QualType QType = TL.getType();
+
+ // Check and compare the base type.
+ if (const IdentifierInfo *Ident = QType.getBaseTypeIdentifier()) {
+ if (Ident->getName() == Name) {
+ Found = true;
+ return false;
+ }
+ } else {
+ // There is no base type, but we still need to check if our variable name
+ // conflicts with a typedef used in the body of the loop.
+ if (QType.getAsString() == Name) {
+ Found = true;
+ return false;
+ }
+ }
+ return true;
+}
Index: cpp11-migrate/LoopConvert/StmtAncestor.h
===================================================================
--- cpp11-migrate/LoopConvert/StmtAncestor.h
+++ cpp11-migrate/LoopConvert/StmtAncestor.h
@@ -194,6 +194,7 @@
bool VisitForStmt(clang::ForStmt *F);
bool VisitNamedDecl(clang::NamedDecl *D);
bool VisitDeclRefExpr(clang::DeclRefExpr *D);
+ bool VisitTypeLoc(clang::TypeLoc TL);
};
#endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_STMT_ANCESTOR_H
Index: cpp11-migrate/LoopConvert/VariableNaming.cpp
===================================================================
--- cpp11-migrate/LoopConvert/VariableNaming.cpp
+++ cpp11-migrate/LoopConvert/VariableNaming.cpp
@@ -57,12 +57,24 @@
return IteratorName;
}
-/// \brief Determines whether or not the the name Symbol exists in LoopContext,
-/// any of its parent contexts, or any of its child statements.
+/// \brief Determines whether or not the the name \a Symbol conflicts with
+/// language keywords or defined macros. Also checks if the name exists in
+/// LoopContext, any of its parent contexts, or any of its child statements.
///
/// We also check to see if the same identifier was generated by this loop
/// converter in a loop nested within SourceStmt.
bool VariableNamer::declarationExists(const StringRef Symbol) {
+ assert(Context != 0 && "Expected an ASTContext");
+ IdentifierInfo &Ident = Context->Idents.get(Symbol);
+
+ // Check if the symbol is not an identifier (ie. is a keyword or alias).
+ if (!isAnyIdentifier(Ident.getTokenID()))
+ return true;
+
+ // Check for conflicting macro definitions.
+ if (Ident.hasMacroDefinition())
+ return true;
+
// Determine if the symbol was generated in a parent context.
for (const Stmt *S = SourceStmt; S != NULL; S = ReverseAST->lookup(S)) {
StmtGeneratedVarNameMap::const_iterator I = GeneratedDecls->find(S);
Index: cpp11-migrate/LoopConvert/VariableNaming.h
===================================================================
--- cpp11-migrate/LoopConvert/VariableNaming.h
+++ cpp11-migrate/LoopConvert/VariableNaming.h
@@ -27,12 +27,13 @@
/// index, if they exist.
class VariableNamer {
public:
- VariableNamer(StmtGeneratedVarNameMap *GeneratedDecls,
- const StmtParentMap *ReverseAST, const clang::Stmt *SourceStmt,
- const clang::VarDecl *OldIndex,
- const clang::VarDecl *TheContainer) :
- GeneratedDecls(GeneratedDecls), ReverseAST(ReverseAST),
- SourceStmt(SourceStmt), OldIndex(OldIndex), TheContainer(TheContainer) { }
+ VariableNamer(
+ StmtGeneratedVarNameMap *GeneratedDecls, const StmtParentMap *ReverseAST,
+ const clang::Stmt *SourceStmt, const clang::VarDecl *OldIndex,
+ const clang::VarDecl *TheContainer, const clang::ASTContext *Context)
+ : GeneratedDecls(GeneratedDecls), ReverseAST(ReverseAST),
+ SourceStmt(SourceStmt), OldIndex(OldIndex), TheContainer(TheContainer),
+ Context(Context) {}
/// \brief Generate a new index name.
///
@@ -47,6 +48,7 @@
const clang::Stmt *SourceStmt;
const clang::VarDecl *OldIndex;
const clang::VarDecl *TheContainer;
+ const clang::ASTContext *Context;
// Determine whether or not a declaration that would conflict with Symbol
// exists in an outer context or in any statement contained in SourceStmt.
Index: test/cpp11-migrate/LoopConvert/naming-alias.cpp
===================================================================
--- /dev/null
+++ test/cpp11-migrate/LoopConvert/naming-alias.cpp
@@ -0,0 +1,52 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: cpp11-migrate -loop-convert %t.cpp -- -I %S/Inputs
+// RUN: FileCheck -input-file=%t.cpp %s
+
+#include "structures.h"
+
+const int N = 10;
+int nums[N];
+int sum = 0;
+
+Val Arr[N];
+Val &func(Val &);
+
+void aliasing() {
+ // The extra blank braces are left as a placeholder for after the variable
+ // declaration is deleted.
+ for (int i = 0; i < N; ++i) {
+ Val &t = Arr[i]; { }
+ int y = t.x;
+ }
+ // CHECK: for (auto & t : Arr)
+ // CHECK-NEXT: { }
+ // CHECK-NEXT: int y = t.x;
+
+ for (int i = 0; i < N; ++i) {
+ Val &t = Arr[i];
+ int y = t.x;
+ int z = Arr[i].x + t.x;
+ }
+ // CHECK: for (auto & [[VAR:[a-z_]+]] : Arr)
+ // CHECK-NEXT: Val &t = [[VAR]];
+ // CHECK-NEXT: int y = t.x;
+ // CHECK-NEXT: int z = [[VAR]].x + t.x;
+
+ for (int i = 0; i < N; ++i) {
+ Val t = Arr[i];
+ int y = t.x;
+ int z = Arr[i].x + t.x;
+ }
+ // CHECK: for (auto & [[VAR:[a-z_]+]] : Arr)
+ // CHECK-NEXT: Val t = [[VAR]];
+ // CHECK-NEXT: int y = t.x;
+ // CHECK-NEXT: int z = [[VAR]].x + t.x;
+
+ for (int i = 0; i < N; ++i) {
+ Val &t = func(Arr[i]);
+ int y = t.x;
+ }
+ // CHECK: for (auto & [[VAR:[a-z_]+]] : Arr)
+ // CHECK-NEXT: Val &t = func([[VAR]]);
+ // CHECK-NEXT: int y = t.x;
+}
Index: test/cpp11-migrate/LoopConvert/naming-conflict.cpp
===================================================================
--- /dev/null
+++ test/cpp11-migrate/LoopConvert/naming-conflict.cpp
@@ -0,0 +1,100 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: cpp11-migrate -loop-convert %t.cpp -- -I %S/Inputs
+// RUN: FileCheck -input-file=%t.cpp %s
+
+#include "structures.h"
+
+#define MAX(a,b) (a > b) ? a : b
+#define DEF 5
+
+const int N = 10;
+int nums[N];
+int sum = 0;
+
+void sameNames() {
+ int nums[N];
+ int num = 0;
+ for (int i = 0; i < N; ++i) {
+ printf("Fibonacci number is %d\n", nums[i]);
+ sum += nums[i] + 2 + num;
+ (void) nums[i];
+ }
+ // CHECK: int num = 0;
+ // CHECK-NEXT: for (auto & [[VAR:[a-z_]+]] : nums)
+ // CHECK-NEXT: printf("Fibonacci number is %d\n", [[VAR]]);
+ // CHECK-NEXT: sum += [[VAR]] + 2 + num;
+ // CHECK-NOT: (void) num;
+ // CHECK: }
+}
+
+void macroConflict() {
+ S MAXs;
+ for (S::const_iterator it = MAXs.begin(), e = MAXs.end(); it != e; ++it) {
+ printf("s has value %d\n", (*it).x);
+ printf("Max of 3 and 5: %d\n", MAX(3,5));
+ }
+ // CHECK: for (auto & MAXs_it : MAXs)
+ // CHECK-NEXT: printf("s has value %d\n", (MAXs_it).x);
+ // CHECK-NEXT: printf("Max of 3 and 5: %d\n", MAX(3,5));
+
+ T DEFs;
+ for (T::iterator it = DEFs.begin(), e = DEFs.end(); it != e; ++it) {
+ if (*it == DEF) {
+ printf("I found %d\n", *it);
+ }
+ }
+ // CHECK: for (auto & DEFs_it : DEFs)
+ // CHECK-NEXT: if (DEFs_it == DEF) {
+ // CHECK-NEXT: printf("I found %d\n", DEFs_it);
+}
+
+void keywordConflict() {
+ T ints;
+ for (T::iterator it = ints.begin(), e = ints.end(); it != e; ++it) {
+ *it = 5;
+ }
+ // CHECK: for (auto & ints_it : ints)
+ // CHECK-NEXT: ints_it = 5;
+
+ U __FUNCTION__s;
+ for (U::iterator it = __FUNCTION__s.begin(), e = __FUNCTION__s.end();
+ it != e; ++it) {
+ int __FUNCTION__s_it = (*it).x + 2;
+ }
+ // CHECK: for (auto & __FUNCTION__s_elem : __FUNCTION__s)
+ // CHECK-NEXT: int __FUNCTION__s_it = (__FUNCTION__s_elem).x + 2;
+}
+
+void typeConflict() {
+ T Vals;
+ // Using the name "Val", although it is the name of an existing struct, is
+ // safe in this loop since it will only exist within this scope.
+ for (T::iterator it = Vals.begin(), e = Vals.end(); it != e; ++it) {
+ }
+ // CHECK: for (auto & Val : Vals)
+
+ // We cannot use the name "Val" in this loop since there is a reference to
+ // it in the body of the loop.
+ for (T::iterator it = Vals.begin(), e = Vals.end(); it != e; ++it) {
+ *it = sizeof(Val);
+ }
+ // CHECK: for (auto & Vals_it : Vals)
+ // CHECK-NEXT: Vals_it = sizeof(Val);
+
+ typedef struct Val TD;
+ U TDs;
+ // Naming the variable "TD" within this loop is safe because the typedef
+ // was never used within the loop.
+ for (U::iterator it = TDs.begin(), e = TDs.end(); it != e; ++it) {
+ }
+ // CHECK: for (auto & TD : TDs)
+
+ // "TD" cannot be used in this loop since the typedef is being used.
+ for (U::iterator it = TDs.begin(), e = TDs.end(); it != e; ++it) {
+ TD V;
+ V.x = 5;
+ }
+ // CHECK: for (auto & TDs_it : TDs)
+ // CHECK-NEXT: TD V;
+ // CHECK-NEXT: V.x = 5;
+}
Index: test/cpp11-migrate/LoopConvert/naming.cpp
===================================================================
--- test/cpp11-migrate/LoopConvert/naming.cpp
+++ /dev/null
@@ -1,67 +0,0 @@
-// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: cpp11-migrate -loop-convert %t.cpp -- -I %S/Inputs
-// RUN: FileCheck -input-file=%t.cpp %s
-
-#include "structures.h"
-
-const int N = 10;
-int nums[N];
-int sum = 0;
-
-Val Arr[N];
-Val &func(Val &);
-
-void aliasing() {
- // The extra blank braces are left as a placeholder for after the variable
- // declaration is deleted.
- for (int i = 0; i < N; ++i) {
- Val &t = Arr[i]; { }
- int y = t.x;
- }
- // CHECK: for (auto & t : Arr)
- // CHECK-NEXT: { }
- // CHECK-NEXT: int y = t.x;
-
- for (int i = 0; i < N; ++i) {
- Val &t = Arr[i];
- int y = t.x;
- int z = Arr[i].x + t.x;
- }
- // CHECK: for (auto & [[VAR:[a-z_]+]] : Arr)
- // CHECK-NEXT: Val &t = [[VAR]];
- // CHECK-NEXT: int y = t.x;
- // CHECK-NEXT: int z = [[VAR]].x + t.x;
-
- for (int i = 0; i < N; ++i) {
- Val t = Arr[i];
- int y = t.x;
- int z = Arr[i].x + t.x;
- }
- // CHECK: for (auto & [[VAR:[a-z_]+]] : Arr)
- // CHECK-NEXT: Val t = [[VAR]];
- // CHECK-NEXT: int y = t.x;
- // CHECK-NEXT: int z = [[VAR]].x + t.x;
-
- for (int i = 0; i < N; ++i) {
- Val &t = func(Arr[i]);
- int y = t.x;
- }
- // CHECK: for (auto & [[VAR:[a-z_]+]] : Arr)
- // CHECK-NEXT: Val &t = func([[VAR]]);
- // CHECK-NEXT: int y = t.x;
-}
-
-void sameNames() {
- int num = 0;
- for (int i = 0; i < N; ++i) {
- printf("Fibonacci number is %d\n", nums[i]);
- sum += nums[i] + 2 + num;
- (void) nums[i];
- }
- // CHECK: int num = 0;
- // CHECK-NEXT: for (auto & [[VAR:[a-z_]+]] : nums)
- // CHECK-NEXT: printf("Fibonacci number is %d\n", [[VAR]]);
- // CHECK-NEXT: sum += [[VAR]] + 2 + num;
- // CHECK-NOT: (void) num;
- // CHECK: }
-}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
