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

Clang Frontend
1

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? :slight_smile:

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

+ 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 :wink:
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 :slight_smile:

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

2

It's a cat ("CAT"). I get it. That said, it shouldn't be in your final commit. :wink:

3

/me headdesks

Cute :slight_smile:

4

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.

loop-converter-1.patch (18.8 KB)

5

+ // A container which holds all allowed usages of TargetVar.
+ UsageResult Usages;

Why "allowed"?

Also, perhaps s/TargetVar/IndexVar/. Perhaps also
s/ForLoopASTVisitor/ForLoopIndexUseVisitor/ or something?

+static StringRef getStringFromRange(SourceManager &SourceMgr,
+ const LangOptions &LangOpts,
+ SourceRange Range) {

Is there a good reason to put LangOpts into the interface here? (can't
we create a default instance inside)?

+// Returns true when two Exprs are the same.
+static bool areSameExpr(ASTContext* Context, const Expr *First,
+ const Expr *Second) {

Here you say "same", below you say "equivalent". I interpret "same"
and "equivalent" as two different things. My guess would be that we
mean equivalent, and I'd rather name the functions consistently.

+// Returns true when the index expression is a declaration reference to
+// TargetVar and the array's base exists.
+static bool isValidSubscriptExpr(const Expr *IndexExpr,
+ const VarDecl *TargetVar,
+ const Expr *Arr) {

Didn't I see that in the other patch? Are the patches overlapping?
Consider /me confused :slight_smile:

+// Determines whether the bound of a for loop condition expression matches
+// TheArray.
+static bool arrayMatchesConditionExpr(ASTContext *Context,
+ const QualType &ArrayType,
+ const Expr *ConditionExpr) {

"TheArray" is not a parameter. Perhaps "is the same as the statically
computable size
of ArrayType".

+ return TraverseStmt(Arr) && TraverseStmt(ASE->getIdx());

Any reason not to call TraverseArraySubscriptExpr on the parent?

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.

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 :slight_smile:

- 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? :slight_smile:

I like isSameValue() much better :slight_smile:

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

Ah, perhaps s/isValidSubscriptExpr/isIndexInSubscriptExpr/ or something?

Because the point is that TargetVar is actually the only thing in the
subscript expression. I think the name should transport that.

6

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.

loop-converter-1.patch (18.9 KB)

loop-converter-complete.patch (100 KB)

7

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.

Wow, 3k LOC :slight_smile:

I think it would help me a lot if you could put that into phabricator :slight_smile:

(if you want to you can use arc for that; see
http://www.phabricator.com/docs/phabricator/article/Arcanist_Quick_Start.html
for the docs; you can also just upload the patch at
http://llvm-reviews.chandlerc.com via Differential -> Create Revision
-> Choose File ...)

Thanks!
/Manuel