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

[Sam Panzer]

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.


On Mon, Aug 13, 2012 at 4:40 AM, Manuel Klimek <klimek at google.com> wrote:

>
> Hi Sam,
>
> really cool stuff! I've changed the subject so we can keep a review
> thread per patch.
>
>
Okay. When we get around to it, I'll create a separate thread for further
patches.


> High-level comment:
> - I personally prefer to put the public stuff first in the class
> definition, as that's what I'm interested in as a user of the class;
> it is also the most important thing about the class; if nobody
> objects, I'd vote that we make that a rule in the tools/extra
> repository.
>

Okay, done. For the next patch too :)


> - I would like to see a few more negative tests; I've added comments
> inline for which parts of the code I'd want to see more tests; my
> general guide-line is that there should be at least one test that
> breaks when you change lines of code in an "easy to get wrong" way...
>
>
Good point. I'll spend a while working on more negative test cases.


> +  if (const ConstantArrayType *CAT = dyn_cast<ConstantArrayType>(T)) { //
> meow
>
> meow what?
>

I completely forgot that this comment was still in there. Removed!


>
> +    // The array's size will always be an APInt representing the length
> as an
> +    // unsigned value, and ConditionExpr may evaluate to a signed value.
> +    // To avoid precision loss, we extend each to be one bit larger than
> the
> +    // largest size, convert ConditionExpr to an unsigned value, and
> compare
> +    // the two unsigned values.
>
> I'd add tests that verify the corner cases here...
> Or, can you just call APSInt::isSameValue() and get rid of the comment
> and the complex code? :)
>
>
I like isSameValue() much better :)


> +bool ForLoopASTVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr
> *ASE) {
> +  Expr *Arr = ASE->getBase();
> +  if (isValidSubscriptExpr(ASE->getIdx(), TargetVar, Arr)) {
> +    const Expr *ArrReduced = Arr->IgnoreParenCasts();
> +    if (!containsExpr(Context, &ContainersIndexed, ArrReduced))
> +      ContainersIndexed.insert(ArrReduced);
> +    Usages.push_back(ASE);
> +    return true;
>
> Why's this only a Usage when isValidSubscriptExpr? (comment might help)
>

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.


>
> +//////////////////// ForLoopASTVisitor functions
>
> I have a strong dislike of ascii art in source code, as I think it's
> not helpful (ForLoopASTVisitor methods are easy to detect by the
> ForLoopASTVisitor:: prefix ;)
>

Okay. I wanted to make sure that I kept the static functions organized.


>
> +  if (!FS || !Context->getSourceManager().isFromMainFile(FS->getForLoc()))
>
> FS cannot be NULL. If you don't trust me, use assert() and report a
> bug if it ever fails ;)
>

Check deleted.


> Is the isFromMainFile part tested anywhere?
>

Good point!
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 *any* header),  configurable from
an option in RefactoringTool or ClangTool. I'll add a test for this now,
though.


>
> +  if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar,
> InitVar))
> +    return;
>
> I'd add negative tests for those...
>

Tests added.


>
> +  if (!BoundExpr)
> +    return;
>
> Again, cannot happen - assert() if you want.
>

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.


>
> +  const ForLoopASTVisitor::ContainerResult &ContainersIndexed =
> +      Finder.getContainersIndexed();
> +  if (ContainersIndexed.size() != 1)
> +    return;
>
> I'd add a negative test for that.
>

Done. Another test that I hadn't included in this patch...


>
> +//===-- llvm/Instruction.h - Instruction class definition -------*-
> C++ -*-===//
>
> Wrong name :)
>

Fixed.


>
> +// This file contains matchers and callbacks for use in migrating C++
> for loops.
> +//
> +//
>
> +//===----------------------------------------------------------------------===//
>
> Remove unnecessary vertical space.
>

Done.


>
> +class ForLoopASTVisitor : public RecursiveASTVisitor<ForLoopASTVisitor> {
> + public :
>
> Why's this in the header? Isn't this an implementation detail of LoopFixer?
>
>
Removed. I guess some of my work process leaked.



> +  // Accessor for OnlyUsedAsIndex
> +  bool isOnlyUsedAsIndex() const { return OnlyUsedAsIndex; }
>
> Unused (outside of the class). Remove?
>

Yep. Another process leakage...


>
> +using std::vector;
> +using std::string;
>
> I'd generally prefer to just spell them out - seems like this is more
> noise than the view extra std::.
>
>
Done.


> Cheers,
> /Manuel
>
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120813/0cbe9960/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-converter-1.patch
Type: application/octet-stream
Size: 19265 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120813/0cbe9960/attachment.obj>