[cfe-dev] [PATCH] For loop migration tool (round 1) - patch 1
Manuel Klimek
klimek at google.com
Thu Aug 16 05:31:20 PDT 2012
+ // 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 :)
+// 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?
On Tue, Aug 14, 2012 at 3:17 AM, Sam Panzer <panzer at google.com> wrote:
>
> 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.
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.
>
>>
>>
>> +//////////////////// 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
>> >
>
>
More information about the cfe-dev
mailing list