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

Sam Panzer reviews at llvm-reviews.chandlerc.com
Tue Aug 21 16:43:01 PDT 2012 

  In response to silvas asking why I didn't reuse static analzer code, I wrote the first version based on a RecursiveASTVisitor, and didn't much of the analysis now present. The suggestions I received mostly pointed me towards Tooling.

  Looking at the static analyzer now, I don't see too much that appears very similar, since I'm mostly working at a higher level than the CFG.


================
Comment at: test/loop-convert/Inputs/structures.h:5
@@ +4,3 @@
+extern "C" {
+extern int printf(const char *restrict, ...);
+}
----------------
Sean Silva wrote:
> I don't think it's legal to use `restrict` in C++, even in `extern "C"`.
This is how several other test files declare printf, in particular, test/SemaCXX/format-strings.cpp.

================
Comment at: loop-convert/LoopMatchers.cpp:21
@@ +20,3 @@
+StatementMatcher makeArrayLoopMatcher() {
+  static StatementMatcher LHSMatcher =
+  expression(ignoringImpCasts(declarationReference(to(
----------------
Sean Silva wrote:
> Why are these static? (are they generating static constructors... ew...).
This was a leftover from the tutorial I wrote, before I made this into a factory function. I missed deleting it - thanks!

================
Comment at: loop-convert/LoopActions.cpp:836
@@ +835,3 @@
+
+  ConfidenceLevel = std::min(ConfidenceLevel, Finder.getConfidenceLevel());
+  // We require that a single array/container be indexed into by LoopVar.
----------------
Sean Silva wrote:
> You seem to have a lot of these bare calls to `std::min()` to compute the `ConfidenceLevel` all over the place. Could you encapsulate this operation and give it a descriptive name?
I'm hesitant to create a class which is responsible for tracking a single enum value and making sure it decreases monotonically. But after doing so, I agree that the client code reads much more easily.

================
Comment at: loop-convert/LoopActions.h:24
@@ +23,3 @@
+namespace loop_migrate {
+using clang::ast_matchers::MatchFinder;
+using clang::ast_matchers::StatementMatcher;
----------------
Manuel Klimek wrote:
> No using in headers...
Missed this one... deleted.

================
Comment at: loop-convert/LoopActions.h:30
@@ +29,3 @@
+  TCK_Risky,
+  TCK_Extra,
+  TCK_Safe
----------------
Manuel Klimek wrote:
> Extra doesn't seem to fit in here (is that "extra safe"?)... Perhaps add a comment?
Richard Smith and I tried to come up with a better name. I settled on TCK_Reasonable for now.

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

================
Comment at: loop-convert/LoopActions.cpp:28
@@ +27,3 @@
+
+// The true return value of ForLoopIndexUseVisitor.
+typedef llvm::SmallVector<Usage, 8> UsageResult;
----------------
Manuel Klimek wrote:
> Wondering what "true" means here: true as in indicates success, or true as in real?
True as in real. I changed the comment to explain that ForLoopIndexUseVisitor exists to compute a UsageResult (and one boolean value).

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

================
Comment at: loop-convert/LoopActions.cpp:58
@@ +57,3 @@
+  /// Accessor for ContainersIndexed.
+  const ContainerResult &getContainersIndexed() const {
+    return ContainersIndexed;
----------------
Manuel Klimek wrote:
> 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.
I had imagined possibly doing an analysis to allow multiple containers if we could determine they were in fact the same object/array. As that's not likely to happen any time soon, done.

I actually need to check that exactly one container is indexed, since zero index
ed containers isn't convertible either (what would it loop over? (unless a range type exists...)).

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

================
Comment at: loop-convert/LoopActions.cpp:88
@@ +87,3 @@
+  bool findUsages(const Stmt *Body) {
+    ConfidenceLevel = TCK_Safe;
+    TraverseStmt(const_cast<Stmt *>(Body));
----------------
Manuel Klimek wrote:
> 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.
This would theoretically allow multiple runs (from the pre-matcher days), but again it's not going to be used now. The new confidence level constructor is called in the ForLoopIndexUseVistor constructor.

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

================
Comment at: loop-convert/LoopActions.cpp:94
@@ +93,3 @@
+ private:
+  bool OnlyUsedAsIndex;
+  ASTContext *Context;
----------------
Manuel Klimek wrote:
> 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
> 
Done.

================
Comment at: loop-convert/LoopActions.cpp:112
@@ +111,3 @@
+  llvm::SmallVector<
+      std::pair<const Expr *, llvm::FoldingSetNodeID>, 16> DependentExprs;
+
----------------
Manuel Klimek wrote:
> DependentExprs could need a comment.
It does, and now has one.

================
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,
----------------
Manuel Klimek wrote:
> 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.
This is partly because I've been putting off trying to deal with macros. I guess this function stays local, then.

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

================
Comment at: loop-convert/LoopMatchers.cpp:17
@@ +16,3 @@
+
+// FIXME: How best to document complicated matcher expressions? They're fairly
+// self-documenting...but there may be some unintuitive parts.
----------------
Sean Silva wrote:
> I think that most of these "matcher factory" functions would benefit from a documentation comment containing:
> 
> * A list of bound variable names (or whatever you call the things captured by `id()` and `.bind()`)
> * A chunk of example code with the part that would be matched called out in some way (maybe surround it with `@@@`, `|||` or some other marker which will clearly stand out)
> 
> Also, for these in particular, I would like to see a brief explanation of how they fit into the larger for-loop conversion process.
> 
This is an excellent idea. In fact, I even caught a bug when explicitly documenting exactly what these matchers should do. Does the set of comments added to the next revision seem like what you were looking for?


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

More information about the cfe-commits mailing list