<br class="">Since this patch was originally put up for review, I had made a few changes and corrections (and generated a fourth patch...). I've attached the updated version.<br><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Aug 13, 2012 at 4:40 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>
Hi Sam,<br>
<br>
really cool stuff! I've changed the subject so we can keep a review<br>
thread per patch.<br>
<br></blockquote><div><br></div><div>Okay. When we get around to it, I'll create a separate thread for further patches.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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></blockquote><div><br></div><div>Okay, done. For the next patch too :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- 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></blockquote><div><br></div><div>Good point. I'll spend a while working on more negative test cases.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (const ConstantArrayType *CAT = dyn_cast<ConstantArrayType>(T)) { // meow<br>
<br>
meow what?<br></blockquote><div><br></div><div>I completely forgot that this comment was still in there. Removed!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ // The array's size will always be an APInt representing the length 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 the<br>
+ // largest size, convert ConditionExpr to an unsigned value, and 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></blockquote><div><br></div><div>I like isSameValue() much better :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+bool ForLoopASTVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *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></blockquote><div><br></div><div>The explanation is partly on the function right below it, but I tried to clarify it more, since it's the root of the strategy for this ASTVisitor.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div><br></div><div>Okay. I wanted to make sure that I kept the static functions organized.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ if (!FS || !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></blockquote><div><br></div><div>Check deleted.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Is the isFromMainFile part tested anywhere?<br></blockquote><div><br></div><div>Good point!</div><div>Actually, this is something that I would hope LibTooling could take care of. I suspect that most refactoring tools shouldn't affect certain kinds of included files (system headers, sometimes <i>any</i> header), configurable from an option in RefactoringTool or ClangTool. I'll add a test for this now, though.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar))<br>
+ return;<br>
<br>
I'd add negative tests for those...<br></blockquote><div><br></div><div>Tests added.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+ if (!BoundExpr)<br>
+ return;<br>
<br>
Again, cannot happen - assert() if you want.<br></blockquote><div><br></div><div>This was actually left in as a hook for later, when I add support for other kinds of loops. I've removed it from the array-only version, though.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></blockquote><div><br></div><div>Done. Another test that I hadn't included in this patch...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+//===-- llvm/Instruction.h - Instruction class definition -------*-<br>
C++ -*-===//<br>
<br>
Wrong name :)<br></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+// This file contains matchers and callbacks for use in migrating C++<br>
for loops.<br>
+//<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
<br>
Remove unnecessary vertical space.<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>
+class ForLoopASTVisitor : public RecursiveASTVisitor<ForLoopASTVisitor> {<br>
+ public :<br>
<br>
Why's this in the header? Isn't this an implementation detail of LoopFixer?<br>
<br></blockquote><div><br></div><div>Removed. I guess some of my work process leaked.<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // Accessor for OnlyUsedAsIndex<br>
+ bool isOnlyUsedAsIndex() const { return OnlyUsedAsIndex; }<br>
<br>
Unused (outside of the class). Remove?<br></blockquote><div><br></div><div>Yep. Another process leakage...<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></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">
Cheers,<br>
/Manuel<br>
<br>
On Sat, Aug 4, 2012 at 1:14 AM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank">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" target="_blank">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>