[cfe-dev] [PATCH] For loop migration tool (round 1) - patch 1
Sam Panzer
panzer at google.com
Thu Aug 16 16:00:58 PDT 2012
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.
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
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120816/7c325311/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-converter-1.patch
Type: application/octet-stream
Size: 19378 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120816/7c325311/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-converter-complete.patch
Type: application/octet-stream
Size: 102648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120816/7c325311/attachment-0001.obj>
More information about the cfe-dev
mailing list