[PATCH] Fix UseAuto replacing declaration lists with new expressions

Edwin Vane edwin.vane at intel.com
Thu May 23 13:09:36 PDT 2013



================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:90
@@ -102,1 +89,3 @@
+  const QualType FirstDeclType = FirstDecl->getType().getCanonicalType();
+  TypeLoc FirsDeclTypeLoc = FirstDecl->getTypeSourceInfo()->getTypeLoc();
 
----------------
This variable is no longer required. Just use the initializer in the one place it's used below.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:109
@@ +108,3 @@
+    // one declaration in the declaration list.
+    if (DI != D->decl_begin()) {
+      // All delcarations should match the same non-decorated type.
----------------
Perhaps:

  if (DI == D->decl_begin())
    continue;

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:118
@@ +117,3 @@
+        SourceLocation SL = Q.getStarLoc();
+        Replace.insert(tooling::Replacement(SM, SL, 1, ""));
+        Q = Q.getNextTypeLoc().getAs<PointerTypeLoc>();
----------------
Replacements should be saved up and applied all at once after the for loops completes just in case the `FirstDeclType != V->getType().getCanonicalType()` test passes and causes the loop to abort. Then we'll end up having some replacements scheduled to be applied when they shouldn't. Your tests missed this because you only have vardecl lists of size 2. You'd only see bad behaviour on size 3 or more. I suggest updating the tests.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:101
@@ -118,1 +100,3 @@
+        cast<CXXNewExpr>(V->getInit()->IgnoreParenImpCasts());
+    assert(NewExpr && "No CXXNewExpr provided");
 
----------------
Perhaps a comment here saying the matcher ensures every vardecl has a CXXNewExpr initializer.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:88
@@ +87,3 @@
+  const VarDecl *FirstDecl = cast<VarDecl>(*D->decl_begin());
+  assert(FirstDecl && "No VarDecl provided");
+  const QualType FirstDeclType = FirstDecl->getType().getCanonicalType();
----------------
Maybe add a comment to say the matcher ensures there's at least one vardecl.


http://llvm-reviews.chandlerc.com/D839



More information about the cfe-commits mailing list