[PATCH] Introducing Use-Auto transform for cpp11-migrate

[Dmitri Gribenko]

  This looks great, and I have mostly style comments.  Please commit with these small changes.

  One major difference between this patch and previous is that this tries to convert only iterators for standard containers.  Is this something you plan to implement later for 'reasonable' risk level?


================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:41
@@ +40,3 @@
+    return;
+  else if ((Construct = dyn_cast<CXXConstructExpr>(E)))
+    // If we ran into an implicit conversion constructor, can't convert.
----------------
(1) You don't need 'else' keyword here.

(2) I'd avoid reusing the 'Construct' variable.  It would be clearer to declare a new one: 'NestedConstruct'.


================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:43
@@ +42,3 @@
+    // If we ran into an implicit conversion constructor, can't convert.
+    if (Construct->getConstructor()->isConvertingConstructor(false))
+      return;
----------------
This checks if a constructor can be used implicitly, not if it actually was (it could be spelled out in the source code).  It is correct, but conservative.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:47
@@ +46,3 @@
+  // Compare canonical types so typedefs don't mess the comparison up.
+  if (D->getType().getCanonicalType() == E->getType().getCanonicalType()) {
+    TypeLoc TL = D->getTypeSourceInfo()->getTypeLoc();
----------------
`Result.Context->hasSameType(...)`, if it looks better to you.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:49
@@ +48,3 @@
+    TypeLoc TL = D->getTypeSourceInfo()->getTypeLoc();
+    CharSourceRange Range(TL.getSourceRange(), true);
+    Replace.insert(tooling::Replacement(SM, Range, "auto"));
----------------
As discussed before `TL.getSourceRange()` covers the identifier being declared, so replacing this will break declarations of pointers to member and non-member functions.  It can not happen now, but it would be helpful to add a comment about this here, because the auto transformation is likely to be improved to cover more cases in future.

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:31-34
@@ +30,6 @@
+/// \code
+///   iterator I = C.begin(); // A
+///   MyType A(42);           // B
+///   MyType A{2};            // C
+///   MyType B;               // D
+/// \endcode
----------------
Could you rename the variables to A, B, C, D?  Having variable names from the same 'namespace' as comments, and having two 'A' variables is a bit confusing...

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:73-75
@@ +72,5 @@
+  for (;;) {
+    if (SugarMatcher.matches(QT, Finder, Builder)) {
+      return true;
+    }
+
----------------
Extra braces (here and below).

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:83
@@ +82,3 @@
+  }
+  return false;
+}
----------------
You can replace `break` with this return.

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:105
@@ +104,3 @@
+  for (unsigned int i = 0;
+       i < sizeof(IteratorNames)/sizeof(IteratorNames[0]);
+       ++i) {
----------------
`array_lengthof(IteratorNames)`

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:149
@@ +148,3 @@
+  for (unsigned int i = 0;
+       i < sizeof(ContainerNames)/sizeof(ContainerNames[0]);
+       ++i) {
----------------
array_lengthof

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:165-166
@@ +164,4 @@
+DeclarationMatcher makeIteratorMatcher() {
+  return varDecl(allOf(hasWrittenNonListInitializer(),
+                   unless(hasType(autoType())),
+                   hasType(
----------------
(1) Please align 'has...' and 'unless', or move 'has...' to the next line.

(2) Does autoType() handle all of 'auto', 'auto &' and 'auto &&', and same with qualifiers?  Could you add tests that we don't turn 'auto &' (and friends) into 'auto'?

================
Comment at: test/cpp11-migrate/UseAuto/Inputs/gen_my_std.h.py:80
@@ +79,3 @@
+    print """
+}} // namespace _1"
+using _1::{0};""".format(ClassName)
----------------
Extra `"`.


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

BRANCH
  iter

ARCANIST PROJECT
  clang-tools-extra