[cfe-commits] [PATCH] C++11 Range-based Loop Converter

[Manuel Klimek]

================
Comment at: loop-convert/StmtAncestor.h:115
@@ +114,3 @@
+  /// some variable declared within ContainingStmt.
+  bool dependsOnOutsideVariable(const Stmt *Body) {
+    DependsOnOutsideVariable = false;
----------------
This one especially could need an example or two in the docs :) I find this to be one of the most complex parts of this code.

================
Comment at: loop-convert/VariableNaming.h:22
@@ +21,3 @@
+namespace clang {
+namespace loop_migrate {
+/// \brief VariableNamer - Create names for generated variables within
----------------
I'd add a whitespace for readability. In general, I find comments starting directly after something else without an empty line to be hard on the eye.

================
Comment at: loop-convert/VariableNaming.h:23
@@ +22,3 @@
+namespace loop_migrate {
+/// \brief VariableNamer - Create names for generated variables within
+/// a particular statement.
----------------
Also, I'd either leave out the VariableNamer here, or create a sentence like: VariableNamer creates names...

================
Comment at: loop-convert/VariableNaming.h:32-35
@@ +31,6 @@
+ public:
+  VariableNamer(ASTContext *Context, StmtGeneratedVarNameMap *GeneratedDecls,
+                const StmtParentMap *ReverseAST, const DeclContext *LoopContext,
+                const Stmt *SourceStmt, const VarDecl *OldIndex,
+                const VarDecl *TheContainer) :
+  Context(Context), GeneratedDecls(GeneratedDecls), ReverseAST(ReverseAST),
----------------
I'm wondering whether there's an abstraction missing for that kind of argument list, but I cannot come up with it currently. Something to keep in the back of our minds.

================
Comment at: loop-convert/VariableNaming.cpp:22-23
@@ +21,4 @@
+
+  // FIXME: Maybe create a class so that this call doesn't need 6 parameters
+  // every time?
+  if (!declarationExists(IteratorName))
----------------
Is that comment outdated?

================
Comment at: loop-convert/VariableNaming.cpp:52
@@ +51,3 @@
+/// converter in a loop nested within SourceStmt.
+bool VariableNamer::declarationExists(const std::string& Symbol) {
+  IdentifierInfo& Identifier = Context->Idents.get(Symbol);
----------------
Any reason not to use StringRef?

================
Comment at: loop-convert/VariableNaming.cpp:57-68
@@ +56,14 @@
+
+  // First, let's check the parent context.
+  // FIXME: lookup() always returns the pair (NULL, NULL) because its
+  // StoredDeclsMap is not initialized (i.e. LookupPtr.getInt() is false inside
+  // of DeclContext::lookup()). Why is this?
+  // NOTE: We work around this by checking when a shadowed declaration is
+  // referenced, rather than now.
+  for (const DeclContext *CurrContext = LoopContext; CurrContext != NULL;
+       CurrContext = CurrContext->getLookupParent()) {
+    DeclContext::lookup_const_result Result = CurrContext->lookup(Name);
+    if (Result.first != Result.second)
+        return true;
+  }
+
----------------
I think I mentioned that in one of the pre-phab review mails, but I'm not sure any more :) So, is this now used, or not?


http://llvm-reviews.chandlerc.com/D19