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

[Manuel Klimek]

  First round of comments, more to come :)


================
Comment at: loop-convert/LoopActions.h:24
@@ +23,3 @@
+namespace loop_migrate {
+using clang::ast_matchers::MatchFinder;
+using clang::ast_matchers::StatementMatcher;
----------------
No using in headers...

================
Comment at: loop-convert/LoopActions.h:30
@@ +29,3 @@
+  TCK_Risky,
+  TCK_Extra,
+  TCK_Safe
----------------
Extra doesn't seem to fit in here (is that "extra safe"?)... Perhaps add a comment?

================
Comment at: loop-convert/LoopActions.h:61
@@ +60,3 @@
+class LoopFixer : public MatchFinder::MatchCallback {
+ private:
+  StmtAncestorASTVisitor *ParentFinder;
----------------
Consistency: public section first...

================
Comment at: loop-convert/LoopActions.cpp:25
@@ +24,3 @@
+  /// what needs to be replaced.
+  SourceRange Range;
+};
----------------
In general, this seems like a design smell - can we instead always use Range, and just set it to E->getSourceRange when IsArrow is false? Perhaps even via a constructor, so this is impossible to use incorrectly?

================
Comment at: loop-convert/LoopActions.cpp:28
@@ +27,3 @@
+
+// The true return value of ForLoopIndexUseVisitor.
+typedef llvm::SmallVector<Usage, 8> UsageResult;
----------------
Wondering what "true" means here: true as in indicates success, or true as in real?

================
Comment at: loop-convert/LoopActions.cpp:40
@@ +39,3 @@
+  ForLoopIndexUseVisitor(ASTContext *Context, const VarDecl *IndexVar,
+                    const VarDecl *EndVar, const Expr *ContainerExpr,
+                    bool ContainerNeedsDereference) :
----------------
Indent.

================
Comment at: loop-convert/LoopActions.cpp:94
@@ +93,3 @@
+ private:
+  bool OnlyUsedAsIndex;
+  ASTContext *Context;
----------------
I'd order this roughly by abstraction level:
1. typedefs
2. methods from most abstract to most specific
3. fields that are set at construction time
4. fields that are changed at runtime


================
Comment at: loop-convert/LoopActions.cpp:58
@@ +57,3 @@
+  /// Accessor for ContainersIndexed.
+  const ContainerResult &getContainersIndexed() const {
+    return ContainersIndexed;
----------------
I'd change the interface to:
bool indexesMultipleContainers();
Expr *getSingleIndexedContainer();
where getSingleIndexedContainer() asserts(!indexesMultipleContainers());

That way more of the "how" is hidden in the class.

================
Comment at: loop-convert/LoopActions.cpp:87
@@ +86,3 @@
+  // ArraySubscriptExpression.
+  bool findUsages(const Stmt *Body) {
+    ConfidenceLevel = TCK_Safe;
----------------
Perhaps call this findAndVerifyUsages; I think that'd better resemble the return value's meaning.

================
Comment at: loop-convert/LoopActions.cpp:88
@@ +87,3 @@
+  bool findUsages(const Stmt *Body) {
+    ConfidenceLevel = TCK_Safe;
+    TraverseStmt(const_cast<Stmt *>(Body));
----------------
It seems strange that we set confidence level here, which would indicate that we can re-run this method, but initialize other values that are changed at runtime only in the constructor. I'm fine with both strategies, I'd just prefer to have a single one consistently done.

================
Comment at: loop-convert/LoopActions.cpp:97
@@ +96,3 @@
+  // A container which holds all usages of IndexVar as the index of
+  // ArraySubscriptExpressions.
+  UsageResult Usages;
----------------
Above the comment makes it look like "ArraySubscriptExpression" would only apply to arrays, and not in general...

================
Comment at: loop-convert/LoopActions.cpp:112
@@ +111,3 @@
+  llvm::SmallVector<
+      std::pair<const Expr *, llvm::FoldingSetNodeID>, 16> DependentExprs;
+
----------------
DependentExprs could need a comment.

================
Comment at: loop-convert/LoopActions.cpp:138
@@ +137,3 @@
+// Obtain the original source code text from a SourceRange.
+// FIXME: Maybe put this somewhere more generally accessible?
+static StringRef getStringFromRange(SourceManager &SourceMgr,
----------------
We're making some assumptions here that would not make this fit a more general place: macros can cause all kind of havoc that needs to be checked for.

================
Comment at: loop-convert/LoopActions.cpp:178
@@ +177,3 @@
+// variable.
+static bool exprIsVariable(const ValueDecl *Target, const Expr *E) {
+  if (!Target || !E)
----------------
Rename to 'exprReferencesVariable'?


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