[cfe-dev] [PATCH] For loop migration tool (round 1) - patch 2

[Manuel Klimek]

High-level comment about "using std::string" and similar: this is not
done anywhere else in clang/llvm, so I'd suggest not to start doing
it...
I again have the feeling that not everything I'd like to be tested is
tested, but this time it's harder for me to put my finger on it :)

+struct LoopFixerArgs {
+  tooling::Replacements *Replace;
+  DeclMap *GeneratedDecls;
+  ReplacedVarsMap *ReplacedVarRanges;
+};

What's the reason to not make those fields in LoopFixer?

+  for (const Stmt *Prev = Curr; Curr != NULL;
+       Prev = Curr, Curr = StmtParents->lookup(Prev))

It would seem simpler to me to write that as:
while(Curr != NULL) {
  ...
  Curr = StmtParents->lookup(Curr);
}

or, if you really prefer for loops (llvm isn't consistent here)
for (; Curr != NULL; Curr = StmtParent->lookup(Curr))

+  /// Run the analysis on Body, and return true iff the expression depends on
+  /// some variable declared within ContainingLoop.
+  bool isExternallyDependent(const Stmt *Body) {

I'm not sure I get the name: why is it "externally dependent" if it
depends on a variable within the loop? What does "externally" mean in
that context?

+  /// Attempts to find any usages of variables name Name in Body, returning
+  /// true when it is only used as an array index. A list of usages and
+  /// the arrays TargetVar indexes are also tracked.

Is that comment correct? From the implementation it looks like we
abort traversal when we find the first match.

+  DeclFinderASTVisitor(const std::string &Name,
+                       const DeclMap *GeneratedDecls) :

Please document what GeneratedDecls is here...

+  friend class RecursiveASTVisitor<DependencyFinderASTVisitor>;

Why's that necessary?

+class DeclFinderASTVisitor : public RecursiveASTVisitor<DeclFinderASTVisitor> {
+ private:
+  const std::string &Name;

I'd strongly vote for making this a copy, as otherwise it's really
easy to introduce errors by handing in temps.

+// Generates the name to be used for an inserted iterator. It reliles on
+// declarationExists() to determine that there are no naming conflicts, and
+// tries to use some hints from the container name and the old index name.
+
+string VariableNamer::createIndexName() {

s/reliles/relies/

Also, put the comment to the method.

+bool VariableNamer::declarationExists(const string& Symbol) {
+  IdentifierInfo& Identifier = Context->Idents.get(Symbol);
+  DeclarationName Name =
+      Context->DeclarationNames.getIdentifier(&Identifier);
+
+  // 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;
+  }
+
+  // Next, iterate through child contexts.
+  for (const Stmt *S = SourceStmt; S != NULL; S = ReverseAST->lookup(S)) {
+    DeclMap::const_iterator I = GeneratedDecls->find(S);
+    if (I != GeneratedDecls->end() && I->second == Symbol)
+      return true;
+  }
+  DeclFinderASTVisitor DeclFinder(Symbol, GeneratedDecls);
+  return DeclFinder.findUsages(SourceStmt);

Are all those return paths tested?




On Sat, Aug 4, 2012 at 1:14 AM, Sam Panzer <panzer at google.com> wrote:
> Here are three patches which constitute most of the work needed for the
> for-loop migration tool. The first contains the contents of the tutorial
> from earlier this week, with extra comments and rebased into
> clang/tools/extra/. The second patch fixes most of the correctness
> assumptions that the naive tool makes (e.g. conflicting edits, name
> conflicts), and the third adds some command-line flags along with a trick
> for eliding a variable declared as a reference to the array access.
>
> Together, they provide a nearly complete converter for array-based loops -
> the three optional features I didn't rebase into the patches involve using
> an explicit type rather than auto, adding const when possible, and making
> the loop variable a non-reference type for non-aggregate types if possible.
>
> There is one potentially problematic assumption that I haven't fixed yet:
> the array expression is assumed not to change, as this becomes difficult to
> verify when the code iterates over a compound expression such as
> graph.nodes->getInputs(). In most cases, the loop will probably be
> convertible if it passes all other checks, so I intend to offer a flag that
> tells the loop converter whether or not to make these usually-okay changes.
>
> I also have two more patches in the works for next week, which add support
> for iterator-based loops and loops over array-like containers.
>
> Thoughts?
>
> -Sam
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>