[cfe-dev] [PATCH] For loop migration tool (round 1) - patch 1
Manuel Klimek
klimek at google.com
Fri Aug 17 06:02:48 PDT 2012
On Fri, Aug 17, 2012 at 1:00 AM, Sam Panzer <panzer at google.com> wrote:
> 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 :)
I think it would help me a lot if you could put that into phabricator :)
(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
> On Thu, Aug 16, 2012 at 5:31 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>
>> + // A container which holds all allowed usages of TargetVar.
>> + UsageResult Usages;
>>
>> Why "allowed"?
>
>
> Maybe "consistent" would be better. For now, I reworded the comment just to
> say 'usages.' I haven't decided if it's useful to track all usages even for
> unconvertable loops.
>
>>
>>
>> Also, perhaps s/TargetVar/IndexVar/.
>
>
> For now, I agree. It's somewhat less clear when IndexVar is generalized to
> be an index or an iterator. I was trying to minimize the friction between
> the array case and the others when breaking up the project into reviewable
> patches, but I can rename everything later :)
>
>>
>> Perhaps also
>> s/ForLoopASTVisitor/ForLoopIndexUseVisitor/ or something?
>
>
> Sounds fine, but still doesn't solve the naming problem for the combined
> index/iterator case. I'm still working on that...
>
>>
>>
>> +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)?
>>
>
> I don't really know how LangOpts is used within the codebase, just that
> Lexer::getSourceText() requires one. I also expect that this is the most
> general useful form of this function, which probably belongs where general
> refactoring tools can use it. What would you recommend for a default
> instance?
>
>>
>> +// 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.
>
>
> Yes, s/same/equivalent/
>
>>
>>
>> +// 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 :)
>
>
> I did some rebasing and other rearrangement between initially sending the
> three patches out (now there are seven...but I think they're more logically
> ordered). I think isValidSubscriptExpr was in the earlier version of this
> patch.
>
>>
>>
>> +// 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".
>>
>
> I thought I had cleaned that up...and it turns out it was in the next patch
> :)git status
>
>
>>
>> + return TraverseStmt(Arr) && TraverseStmt(ASE->getIdx());
>>
>> Any reason not to call TraverseArraySubscriptExpr on the parent?
>
>
> No. I had tried calling TraverseStmt and received a stack overflow...but
> TraverseAraySubscriptExpr is the right answer.
>
>>
>>
>>
>> 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.
>
>
> Makes sense. I've renamed it to isIndexInSubscriptExpr as you suggest, and
> I'll come up with similar names for the later cases.
>
>>
>>
>> >
>> >>
>> >>
>> >> +//////////////////// 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