<br><div class="gmail_extra">Here's my next attempt!<br><br><div class="gmail_quote">On Mon, Aug 13, 2012 at 6:45 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
High-level comment about "using std::string" and similar: this is not<br>
done anywhere else in clang/llvm, so I'd suggest not to start doing<br>
it...<br>
I again have the feeling that not everything I'd like to be tested is<br>
tested, but this time it's harder for me to put my finger on it :)<br></blockquote><div><br></div><div>That's partly because I hadn't included all the tests that could be added at this point. They're in <a href="http://now.gg">now.gg</a></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+struct LoopFixerArgs {<br>
+ tooling::Replacements *Replace;<br>
+ DeclMap *GeneratedDecls;<br>
+ ReplacedVarsMap *ReplacedVarRanges;<br>
+};<br>
<br>
What's the reason to not make those fields in LoopFixer?<br></blockquote><div><br></div><div>The only reason apparent in this patch is to keep doConversion static within LoopActions.cpp, though in another few patches I reuse this struct across three LoopFixers (one per kind of convertible loop). Is there a better way to arrange this? (LoopFixerArgs ends up with 10 parameters by the end.)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ for (const Stmt *Prev = Curr; Curr != NULL;<br>
+ Prev = Curr, Curr = StmtParents->lookup(Prev))<br>
<br>
It would seem simpler to me to write that as:<br>
while(Curr != NULL) {<br>
...<br>
Curr = StmtParents->lookup(Curr);<br>
}<br></blockquote><div><br></div><div>Agreed. Switched to a while loop.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
or, if you really prefer for loops (llvm isn't consistent here)<br>
for (; Curr != NULL; Curr = StmtParent->lookup(Curr))<br>
<br>
+ /// Run the analysis on Body, and return true iff the expression depends on<br>
+ /// some variable declared within ContainingLoop.<br>
+ bool isExternallyDependent(const Stmt *Body) {<br><br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm not sure I get the name: why is it "externally dependent" if it<br>
depends on a variable within the loop? What does "externally" mean in<br>
that context?<br></blockquote><div><br></div><div>It's external to ContainingLoop. A full description would be "expression has a free variable in the lexical context of the specified loop," which would mean that hoisting Body outside of ContainingLoop would result in a syntactic (variable not defined) or semantic (identifier refers to a different variable) error. I'm going with dependsOnOutsideVariable for now. Is hasFreeVariableInStmt a better name?<br>
</div><div> </div><div>While I was at it, I also renamed ContainingLoop to ContainingStmt.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ /// Attempts to find any usages of variables name Name in Body, returning<br>
+ /// true when it is only used as an array index. A list of usages and<br>
+ /// the arrays TargetVar indexes are also tracked.<br>
<br>
Is that comment correct? From the implementation it looks like we<br>
abort traversal when we find the first match.<br></blockquote><div><br></div><div>The comment was way off after the first line. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ DeclFinderASTVisitor(const std::string &Name,<br>
+ const DeclMap *GeneratedDecls) :<br>
<br>
Please document what GeneratedDecls is here...<br></blockquote><div><br></div><div>It was somewhat opaque; comments added. That's what I use to track the loop-to-generated-variable-name mapping.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ friend class RecursiveASTVisitor<DependencyFinderASTVisitor>;<br>
<br>
Why's that necessary?<br></blockquote><div><br></div><div>CRTP + private overrides. This way, the VisitXXX functions don't have to be public.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+class DeclFinderASTVisitor : public RecursiveASTVisitor<DeclFinderASTVisitor> {<br>
+ private:<br>
+ const std::string &Name;<br>
<br>
I'd strongly vote for making this a copy, as otherwise it's really<br>
easy to introduce errors by handing in temps.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+// Generates the name to be used for an inserted iterator. It reliles on<br>
+// declarationExists() to determine that there are no naming conflicts, and<br>
+// tries to use some hints from the container name and the old index name.<br>
+<br>
+string VariableNamer::createIndexName() {<br>
<br>
s/reliles/relies/<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, put the comment to the method.<br></blockquote><div><br></div><div>Do you mean move the comment to the .h file? Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+bool VariableNamer::declarationExists(const string& Symbol) {<br>
+ IdentifierInfo& Identifier = Context->Idents.get(Symbol);<br>
+ DeclarationName Name =<br>
+ Context->DeclarationNames.getIdentifier(&Identifier);<br>
+<br>
+ // First, let's check the parent context.<br>
+ // FIXME: lookup() always returns the pair (NULL, NULL) because its<br>
+ // StoredDeclsMap is not initialized (i.e. LookupPtr.getInt() is false inside<br>
+ // of DeclContext::lookup()). Why is this?<br>
+ // NOTE: We work around this by checking when a shadowed declaration is<br>
+ // referenced, rather than now.<br>
+ for (const DeclContext *CurrContext = LoopContext; CurrContext != NULL;<br>
+ CurrContext = CurrContext->getLookupParent()) {<br>
+ DeclContext::lookup_const_result Result = CurrContext->lookup(Name);<br>
+ if (Result.first != Result.second)<br>
+ return true;<br>
+ }<br>
+<br>
+ // Next, iterate through child contexts.<br></blockquote><div><br></div><div>The above comment was incorrect. Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ for (const Stmt *S = SourceStmt; S != NULL; S = ReverseAST->lookup(S)) {<br>
+ DeclMap::const_iterator I = GeneratedDecls->find(S);<br>
+ if (I != GeneratedDecls->end() && I->second == Symbol)<br>
+ return true;<br>
+ }<br>
+ DeclFinderASTVisitor DeclFinder(Symbol, GeneratedDecls);<br>
+ return DeclFinder.findUsages(SourceStmt);<br>
<br>
Are all those return paths tested?<br>
<br>
<br></blockquote><div><br></div><div>The first one doesn't fire on my tests due to the problem documented at the top of this function. The others have tests in loop-convert-nesting.cpp and loop-convert-naming.cpp (I didn't attach loop-convert-naming.cpp to the original patch.), though they needed fixing due to a FileCheck quirk.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
On Sat, Aug 4, 2012 at 1:14 AM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
> Here are three patches which constitute most of the work needed for the<br>
> for-loop migration tool. The first contains the contents of the tutorial<br>
> from earlier this week, with extra comments and rebased into<br>
> clang/tools/extra/. The second patch fixes most of the correctness<br>
> assumptions that the naive tool makes (e.g. conflicting edits, name<br>
> conflicts), and the third adds some command-line flags along with a trick<br>
> for eliding a variable declared as a reference to the array access.<br>
><br>
> Together, they provide a nearly complete converter for array-based loops -<br>
> the three optional features I didn't rebase into the patches involve using<br>
> an explicit type rather than auto, adding const when possible, and making<br>
> the loop variable a non-reference type for non-aggregate types if possible.<br>
><br>
> There is one potentially problematic assumption that I haven't fixed yet:<br>
> the array expression is assumed not to change, as this becomes difficult to<br>
> verify when the code iterates over a compound expression such as<br>
> graph.nodes->getInputs(). In most cases, the loop will probably be<br>
> convertible if it passes all other checks, so I intend to offer a flag that<br>
> tells the loop converter whether or not to make these usually-okay changes.<br>
><br>
> I also have two more patches in the works for next week, which add support<br>
> for iterator-based loops and loops over array-like containers.<br>
><br>
> Thoughts?<br>
><br>
> -Sam<br>
><br>
><br>
> _______________________________________________<br>
> cfe-dev mailing list<br>
> <a href="mailto:cfe-dev@cs.uiuc.edu">cfe-dev@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a><br>
><br>
</blockquote></div><br></div>