Here's my next attempt! I have also attached the complete patch (diff between clang/tools/extra and my HEAD), which you requested in another thread for context. I will try to get it on my GitHub as well.<br><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Aug 16, 2012 at 5:31 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>
+ // A container which holds all allowed usages of TargetVar.<br>
+ UsageResult Usages;<br>
<br>
Why "allowed"?<br></blockquote><div><br></div><div>Maybe "consistent" would be better. For now, I reworded the comment just to say 'usages.' I haven't decided if it's useful to track all usages even for unconvertable loops.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, perhaps s/TargetVar/IndexVar/.</blockquote><div><br></div><div>For now, I agree. It's somewhat less clear when IndexVar is generalized to be an index or an iterator. I was trying to minimize the friction between the array case and the others when breaking up the project into reviewable patches, but I can rename everything later :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Perhaps also<br>
s/ForLoopASTVisitor/ForLoopIndexUseVisitor/ or something?<br></blockquote><div><br></div><div>Sounds fine, but still doesn't solve the naming problem for the combined index/iterator case. I'm still working on that...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+static StringRef getStringFromRange(SourceManager &SourceMgr,<br>
+ const LangOptions &LangOpts,<br>
+ SourceRange Range) {<br>
<br>
Is there a good reason to put LangOpts into the interface here? (can't<br>
we create a default instance inside)?<br>
<br></blockquote><div><br></div><div>I don't really know how LangOpts is used within the codebase, just that Lexer::getSourceText() requires one. I also expect that this is the most general useful form of this function, which probably belongs where general refactoring tools can use it. What would you recommend for a default instance?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+// Returns true when two Exprs are the same.<br>
+static bool areSameExpr(ASTContext* Context, const Expr *First,<br>
+ const Expr *Second) {<br>
<br>
Here you say "same", below you say "equivalent". I interpret "same"<br>
and "equivalent" as two different things. My guess would be that we<br>
mean equivalent, and I'd rather name the functions consistently.<br></blockquote><div><br></div><div>Yes, s/same/equivalent/</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+// Returns true when the index expression is a declaration reference to<br>
+// TargetVar and the array's base exists.<br>
+static bool isValidSubscriptExpr(const Expr *IndexExpr,<br>
+ const VarDecl *TargetVar,<br>
+ const Expr *Arr) {<br>
<br>
Didn't I see that in the other patch? Are the patches overlapping?<br>
Consider /me confused :)<br></blockquote><div><br></div><div>I did some rebasing and other rearrangement between initially sending the three patches out (now there are seven...but I think they're more logically ordered). I think isValidSubscriptExpr was in the earlier version of this patch.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+// Determines whether the bound of a for loop condition expression matches<br>
+// TheArray.<br>
+static bool arrayMatchesConditionExpr(ASTContext *Context,<br>
+ const QualType &ArrayType,<br>
+ const Expr *ConditionExpr) {<br>
<br>
"TheArray" is not a parameter. Perhaps "is the same as the statically<br>
computable size<br>
of ArrayType".<br>
<br></blockquote><div><br></div><div>I thought I had cleaned that up...and it turns out it was in the next patch :)git status</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ return TraverseStmt(Arr) && TraverseStmt(ASE->getIdx());<br>
<br>
Any reason not to call TraverseArraySubscriptExpr on the parent?<br></blockquote><div><br></div><div>No. I had tried calling TraverseStmt and received a stack overflow...but TraverseAraySubscriptExpr is the right answer.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<br>
On Tue, Aug 14, 2012 at 3:17 AM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
><br>
> Since this patch was originally put up for review, I had made a few changes<br>
> and corrections (and generated a fourth patch...). I've attached the updated<br>
> version.<br>
><br>
><br>
> On Mon, Aug 13, 2012 at 4:40 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>><br>
>><br>
>> Hi Sam,<br>
>><br>
>> really cool stuff! I've changed the subject so we can keep a review<br>
>> thread per patch.<br>
>><br>
><br>
> Okay. When we get around to it, I'll create a separate thread for further<br>
> patches.<br>
><br>
>><br>
>> High-level comment:<br>
>> - I personally prefer to put the public stuff first in the class<br>
>> definition, as that's what I'm interested in as a user of the class;<br>
>> it is also the most important thing about the class; if nobody<br>
>> objects, I'd vote that we make that a rule in the tools/extra<br>
>> repository.<br>
><br>
><br>
> Okay, done. For the next patch too :)<br>
><br>
>><br>
>> - I would like to see a few more negative tests; I've added comments<br>
>> inline for which parts of the code I'd want to see more tests; my<br>
>> general guide-line is that there should be at least one test that<br>
>> breaks when you change lines of code in an "easy to get wrong" way...<br>
>><br>
><br>
> Good point. I'll spend a while working on more negative test cases.<br>
><br>
>><br>
>> + if (const ConstantArrayType *CAT = dyn_cast<ConstantArrayType>(T)) { //<br>
>> meow<br>
>><br>
>> meow what?<br>
><br>
><br>
> I completely forgot that this comment was still in there. Removed!<br>
><br>
>><br>
>><br>
>> + // The array's size will always be an APInt representing the length<br>
>> as an<br>
>> + // unsigned value, and ConditionExpr may evaluate to a signed value.<br>
>> + // To avoid precision loss, we extend each to be one bit larger than<br>
>> the<br>
>> + // largest size, convert ConditionExpr to an unsigned value, and<br>
>> compare<br>
>> + // the two unsigned values.<br>
>><br>
>> I'd add tests that verify the corner cases here...<br>
>> Or, can you just call APSInt::isSameValue() and get rid of the comment<br>
>> and the complex code? :)<br>
>><br>
><br>
> I like isSameValue() much better :)<br>
><br>
>><br>
>> +bool ForLoopASTVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr<br>
>> *ASE) {<br>
>> + Expr *Arr = ASE->getBase();<br>
>> + if (isValidSubscriptExpr(ASE->getIdx(), TargetVar, Arr)) {<br>
>> + const Expr *ArrReduced = Arr->IgnoreParenCasts();<br>
>> + if (!containsExpr(Context, &ContainersIndexed, ArrReduced))<br>
>> + ContainersIndexed.insert(ArrReduced);<br>
>> + Usages.push_back(ASE);<br>
>> + return true;<br>
>><br>
>> Why's this only a Usage when isValidSubscriptExpr? (comment might help)<br>
><br>
><br>
> The explanation is partly on the function right below it, but I tried to<br>
> clarify it more, since it's the root of the strategy for this ASTVisitor.<br>
<br>
</div></div>Ah, perhaps s/isValidSubscriptExpr/isIndexInSubscriptExpr/ or something? </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Because the point is that TargetVar is actually the only thing in the<br>
subscript expression. I think the name should transport that.<br></blockquote><div><br></div><div>Makes sense. I've renamed it to isIndexInSubscriptExpr as you suggest, and I'll come up with similar names for the later cases.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>><br>
>> +//////////////////// ForLoopASTVisitor functions<br>
>><br>
>> I have a strong dislike of ascii art in source code, as I think it's<br>
>> not helpful (ForLoopASTVisitor methods are easy to detect by the<br>
>> ForLoopASTVisitor:: prefix ;)<br>
><br>
><br>
> Okay. I wanted to make sure that I kept the static functions organized.<br>
><br>
>><br>
>><br>
>> + if (!FS ||<br>
>> !Context->getSourceManager().isFromMainFile(FS->getForLoc()))<br>
>><br>
>> FS cannot be NULL. If you don't trust me, use assert() and report a<br>
>> bug if it ever fails ;)<br>
><br>
><br>
> Check deleted.<br>
><br>
>><br>
>> Is the isFromMainFile part tested anywhere?<br>
><br>
><br>
> Good point!<br>
> Actually, this is something that I would hope LibTooling could take care of.<br>
> I suspect that most refactoring tools shouldn't affect certain kinds of<br>
> included files (system headers, sometimes any header), configurable from an<br>
> option in RefactoringTool or ClangTool. I'll add a test for this now,<br>
> though.<br>
><br>
>><br>
>><br>
>> + if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar,<br>
>> InitVar))<br>
>> + return;<br>
>><br>
>> I'd add negative tests for those...<br>
><br>
><br>
> Tests added.<br>
><br>
>><br>
>><br>
>> + if (!BoundExpr)<br>
>> + return;<br>
>><br>
>> Again, cannot happen - assert() if you want.<br>
><br>
><br>
> This was actually left in as a hook for later, when I add support for other<br>
> kinds of loops. I've removed it from the array-only version, though.<br>
><br>
>><br>
>><br>
>> + const ForLoopASTVisitor::ContainerResult &ContainersIndexed =<br>
>> + Finder.getContainersIndexed();<br>
>> + if (ContainersIndexed.size() != 1)<br>
>> + return;<br>
>><br>
>> I'd add a negative test for that.<br>
><br>
><br>
> Done. Another test that I hadn't included in this patch...<br>
><br>
>><br>
>><br>
>> +//===-- llvm/Instruction.h - Instruction class definition -------*-<br>
>> C++ -*-===//<br>
>><br>
>> Wrong name :)<br>
><br>
><br>
> Fixed.<br>
><br>
>><br>
>><br>
>> +// This file contains matchers and callbacks for use in migrating C++<br>
>> for loops.<br>
>> +//<br>
>> +//<br>
>><br>
>> +//===----------------------------------------------------------------------===//<br>
>><br>
>> Remove unnecessary vertical space.<br>
><br>
><br>
> Done.<br>
><br>
>><br>
>><br>
>> +class ForLoopASTVisitor : public RecursiveASTVisitor<ForLoopASTVisitor> {<br>
>> + public :<br>
>><br>
>> Why's this in the header? Isn't this an implementation detail of<br>
>> LoopFixer?<br>
>><br>
><br>
> Removed. I guess some of my work process leaked.<br>
><br>
><br>
>><br>
>> + // Accessor for OnlyUsedAsIndex<br>
>> + bool isOnlyUsedAsIndex() const { return OnlyUsedAsIndex; }<br>
>><br>
>> Unused (outside of the class). Remove?<br>
><br>
><br>
> Yep. Another process leakage...<br>
><br>
>><br>
>><br>
>> +using std::vector;<br>
>> +using std::string;<br>
>><br>
>> I'd generally prefer to just spell them out - seems like this is more<br>
>> noise than the view extra std::.<br>
>><br>
><br>
> Done.<br>
><br>
>><br>
>> Cheers,<br>
>> /Manuel<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<br>
>> > 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>
>> > -<br>
>> > the three optional features I didn't rebase into the patches involve<br>
>> > using<br>
>> > an explicit type rather than auto, adding const when possible, and<br>
>> > making<br>
>> > the loop variable a non-reference type for non-aggregate types if<br>
>> > possible.<br>
>> ><br>
>> > There is one potentially problematic assumption that I haven't fixed<br>
>> > yet:<br>
>> > the array expression is assumed not to change, as this becomes difficult<br>
>> > 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<br>
>> > that<br>
>> > tells the loop converter whether or not to make these usually-okay<br>
>> > changes.<br>
>> ><br>
>> > I also have two more patches in the works for next week, which add<br>
>> > 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>
><br>
><br>
</div></div></blockquote></div><br></div>