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

[Manuel Klimek]

Hi Sam,

really cool stuff! I've changed the subject so we can keep a review
thread per patch.

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.
- 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...

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

meow what?

+    // 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? :)

+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)

+//////////////////// 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 ;)

+  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 ;)
Is the isFromMainFile part tested anywhere?

+  if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar))
+    return;

I'd add negative tests for those...

+  if (!BoundExpr)
+    return;

Again, cannot happen - assert() if you want.

+  const ForLoopASTVisitor::ContainerResult &ContainersIndexed =
+      Finder.getContainersIndexed();
+  if (ContainersIndexed.size() != 1)
+    return;

I'd add a negative test for that.

+//===-- llvm/Instruction.h - Instruction class definition -------*-
C++ -*-===//

Wrong name :)

+// This file contains matchers and callbacks for use in migrating C++
for loops.
+//
+//
+//===----------------------------------------------------------------------===//

Remove unnecessary vertical space.

+class ForLoopASTVisitor : public RecursiveASTVisitor<ForLoopASTVisitor> {
+ public :

Why's this in the header? Isn't this an implementation detail of LoopFixer?

+  // Accessor for OnlyUsedAsIndex
+  bool isOnlyUsedAsIndex() const { return OnlyUsedAsIndex; }

Unused (outside of the class). Remove?

+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::.

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
>