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

[Manuel Klimek]

  One more flight of comments, before switching to the latest diff :)


================
Comment at: loop-convert/LoopActions.cpp:228
@@ +227,3 @@
+
+/// \brief If the expression is a dereference or call to operator*(), return the
+/// operand. Otherwise, returns NULL.
----------------
s/return/returns/ for consistency.

================
Comment at: loop-convert/LoopActions.cpp:255
@@ +254,3 @@
+/// \brief Returns true when the index expression is a declaration reference to
+/// IndexVar and the array's base exists.
+///
----------------
The "and the array's base exists" part seems pretty arbitrary. 

================
Comment at: loop-convert/LoopActions.cpp:299
@@ +298,3 @@
+  if (areSameExpr(Context, SourceExpr->IgnoreParenImpCasts(),
+                   Obj->IgnoreParenImpCasts()))
+    return true;
----------------
Indent.

================
Comment at: loop-convert/LoopActions.cpp:327
@@ +326,3 @@
+///
+/// Note that this is the
+/// only isValidXXX function that confirms that the opcode is correct, as there
----------------
Formatting.

================
Comment at: loop-convert/LoopActions.cpp:328-330
@@ +327,5 @@
+/// Note that this is the
+/// only isValidXXX function that confirms that the opcode is correct, as there
+/// is only one way to trigger this case (namely, the builtin operator*).
+/// For example, if the index variable is `index`, returns true for
+///   *index
----------------
I'm not sure I understand what you're getting at. This seems to be a hint at the overall strategy of the various isValid* functions (this is not named isValid* btw, so this is further confusing). 

================
Comment at: loop-convert/LoopActions.cpp:354
@@ +353,3 @@
+///   }
+static bool isAliasDecl(const Decl *TheDecl, const VarDecl *TargetDecl) {
+  const VarDecl *VDecl = dyn_cast<VarDecl>(TheDecl);
----------------
TargetDecl is really confusing here. I thought you meant the target of the initialization, but that's TheDecl :)

Perhaps s/TargetDecl/IndexVar/. In general, naming those things consistently would help.

================
Comment at: loop-convert/LoopActions.cpp:412
@@ +411,3 @@
+
+/// Permitted usages here are dereferences of pointer types.
+///
----------------
This showing up as the overview comment for the method in doxygen seems strange...

================
Comment at: loop-convert/LoopActions.cpp:525
@@ +524,3 @@
+/// operator[].
+/// permitted in iterator loops, so we note them for conversion.
+///
----------------
Leftover sentence?

================
Comment at: loop-convert/LoopActions.cpp:715
@@ +714,3 @@
+
+/// \brief Determine whether VDecl appears to be an initializing an iterator.
+///
----------------
There is no parameter named VDecl...

================
Comment at: loop-convert/LoopActions.cpp:748
@@ +747,3 @@
+  const Expr *BeginInitExpr = BeginVar->getInit();
+  const Expr *EndInitExpr = EndVar ? EndVar->getInit() : EndExpr;
+
----------------
Please document this in the function comment.

================
Comment at: loop-convert/LoopActions.cpp:759
@@ +758,3 @@
+      return NULL;
+  const Expr *EndSourceExpr = getContainerFromInitializer(EndInitExpr,
+                                                          /*IsBegin=*/false,
----------------
The asymmetry between the name EndSourceExpr and ContainerExpr is confusing.
Perhaps BeginContainerExpr and EndContainerExpr?

================
Comment at: loop-convert/LoopActions.cpp:779
@@ +778,3 @@
+
+  if (!Context->getSourceManager().isFromMainFile(TheLoop->getForLoc()))
+    return;
----------------
Any reasons we don't want to make those transformations in headers?
(The strategy I've used so far was regexps on the path)

================
Comment at: loop-convert/LoopActions.cpp:818
@@ +817,3 @@
+  ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr,
+                           ContainerNeedsDereference);
+
----------------
Indent.

================
Comment at: loop-convert/LoopActions.cpp:774
@@ +773,3 @@
+/// matchers are convertible, printing information about the loops if so.
+void LoopFixer::run(const MatchFinder::MatchResult &Result) {
+  TranslationConfidenceKind ConfidenceLevel = TCK_Safe;
----------------
Really long function, would be great to find a way to split this up.

================
Comment at: loop-convert/LoopActions.cpp:858
@@ +857,3 @@
+  // implementation and rely on conflicting edit detection instead.
+  if (ReplacedVarRanges->count(TheLoop)) {
+    ++*DeferredChanges;
----------------
When I came to this condition, I wondered where we had already tried to create replacements in this run() call. I think that's a general problem here - there's too many different things going on on the same level.

================
Comment at: loop-convert/StmtAncestor.h:47
@@ +46,3 @@
+  /// work unless RunEvenIfNotEmpty is set to true.
+  void gatherAncestors(const TranslationUnitDecl *TUD, bool RunEvenIfNotEmpty) {
+    if (RunEvenIfNotEmpty || StmtAncestors.empty()) {
----------------
Seems to be only called once - why the bool parameter?

================
Comment at: loop-convert/LoopConvert.cpp:81
@@ +80,3 @@
+  if (int result = SyntaxTool.run(
+      newFrontendActionFactory<clang::SyntaxOnlyAction>())) {
+    llvm::errs() << "Error compiling files.\n";
----------------
I think that indent is bad, as it looks like there's two things on the same level.

================
Comment at: loop-convert/LoopMatchers.h:34
@@ +33,3 @@
+ast_matchers::StatementMatcher makeIteratorLoopMatcher();
+ast_matchers::StatementMatcher makePseudoArrayLoopMatcher();
+} //namespace loop_migrate
----------------
Please insert newline.

================
Comment at: loop-convert/LoopMatchers.cpp:22
@@ +21,3 @@
+  static StatementMatcher LHSMatcher =
+  expression(ignoringImpCasts(declarationReference(to(
+      variable(hasType(isInteger())).bind(ConditionVarName)))));
----------------
+1 on that they shouldn't be static. Also, indent.

================
Comment at: loop-convert/LoopMatchers.cpp:28
@@ +27,3 @@
+  return id(LoopName, forStmt(
+      hasLoopInit(declarationStatement(hasSingleDecl(id(InitVarName, variable(
+          hasInitializer(ignoringImpCasts(integerLiteral(equals(0))))))))),
----------------
I'd only use one binding concept (.bind()), as it makes it much easier to see at a glance what is being bound.

================
Comment at: loop-convert/StmtAncestor.h:21-32
@@ +20,14 @@
+
+/// A map used to walk the AST in reverse: maps child Stmt to parent Stmt.
+typedef llvm::DenseMap<const Stmt*, const Stmt*> StmtParentMap;
+/// A map used to walk the AST in reverse:
+///  maps VarDecl to the to parent DeclStmt.
+typedef llvm::DenseMap<const VarDecl*, const DeclStmt*> DeclParentMap;
+/// A map used to track which variables have been removed by a refactoring pass.
+/// It maps the parent ForStmt to the removed index variable's VarDecl.
+typedef llvm::DenseMap<const ForStmt*, const VarDecl *> ReplacedVarsMap;
+/// A map used to remember the variable names generated in a Stmt
+typedef llvm::DenseMap<const Stmt*, std::string> StmtGeneratedVarNameMap;
+/// A vector used to store the AST subtrees of an Expr.
+typedef llvm::SmallVector<const Expr *, 16> ComponentVector;
+
----------------
I think adding some newlines would make this easier to read.

================
Comment at: loop-convert/StmtAncestor.h:48
@@ +47,3 @@
+  void gatherAncestors(const TranslationUnitDecl *TUD, bool RunEvenIfNotEmpty) {
+    if (RunEvenIfNotEmpty || StmtAncestors.empty()) {
+      TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
----------------
Can this ever be called twice on the same object? why not assert(StmtAncestors.empty())?


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