================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
