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

[Manuel Klimek]

+// Determines whether the given Decl defines an alias to the given variable.
+static bool isAliasDecl(const Decl *TheDecl, const VarDecl *TargetDecl) {

The name of this function confuses me: where's the aliasing? If I read
it correctly, it checks that TheDecl has an initializer that is an
array subscript expression for the array in question. Now that I read
it I'm also not sure I like the name isValidSubscriptExpr, as that is
way to broad to make it obvious what it actually checks for. I also
don't remember whether I already mentioned that in one of the other
patches.

I actually like the comment below where you say "refers to the loop
element". referencesLoopElement() perhaps?

+  Args->AcceptedChanges++;

Prefer pre-inc for consistency.

+  int NumArgs = argc + 1;
+  const char **Args = new const char *[NumArgs];
+  for (int I = 0; I < argc; ++I)
+    Args[I] = argv[I];
+  Args[NumArgs - 1] = "-std=c++11";

This doesn't work if one is not using the FixedCompilationDatabase, as
far as I can tell... I'm confused about what you're trying to do
here...

+  delete[] Args;

I really dislike allocation and deallocation being so far apart.

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
>