[PATCH] Introducing initial UseAuto transform

[Edwin Vane]

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:50-51
@@ +49,4 @@
+
+  // Compare canonical types so typedefs don't mess the comparison up.
+  if (D->getType().getCanonicalType() == E->getType().getCanonicalType()) {
+    TypeLoc TL = D->getTypeSourceInfo()->getTypeLoc();
----------------
Dmitri Gribenko wrote:
> I think we do want to compare sugared types (unwrapping the sugar until we hit std::somethig?), as per note about iterator types below.
Can you tell me more about this? Sugar just means different names for the same thing right? My understanding of 'canonical' was basically the bottom-most layer of sugar. Should I do something different?

================
Comment at: cpp11-migrate/UseAuto/UseAutoMatchers.cpp:33-35
@@ +32,5 @@
+/// \code
+///   iterator I = C.begin(); // A
+///   MyType A{2};            // B
+///   MyType B;               // C
+/// \endcode
----------------
Dmitri Gribenko wrote:
> What about `MyType A(42)`?
Added this case to the comment.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:53-54
@@ +52,4 @@
+    TypeLoc TL = D->getTypeSourceInfo()->getTypeLoc();
+    CharSourceRange Range(TL.getSourceRange(), true);
+    Replace.insert(tooling::Replacement(SM, Range, "auto"));
+    ++AcceptedChanges;
----------------
Dmitri Gribenko wrote:
> Edwin Vane wrote:
> > Dmitri Gribenko wrote:
> > > Please note, that while this works for iterators, it will not work in general, for example for function pointers or arrays, or pointers to arrays.
> > By 'this' do you mean the matcher/callback as a whole? If so, that's by design. As per your note about iterators and how to identify them I purposefully targeted only std iterators which, according to the spec, do either have these type traits directly or because they were inherited from std::iterator. If custom iterators exist in the wild which inherit from std::iterator then they get the transform for free. I figured this will handle the bulk of the most useful cases.
> By 'this' I mean replacing the type using getSourceRange on TypeLoc.  It will not work correctly for function pointer types spelled directly -- `void (*f)(int)` -- the SourceRange will be the whole declaration, including the identifier `f`.
> 
> No, even iterators for standard containers are not required to have those members.  One should use std::iterator_traits to use those traits.  For example, if vector::iterator is just a plain pointer `T*`, then a standard specialization of iterator_traits<T*> will take care of that, while using member references is an error.
> 
> Of course, I see how checking if std::iterator is a parent class, helps to easily identify types that are iterators.  But what I'm saying is that there are other cases where this is not sufficient (and there are standard libraries that fall into these cases).
I added a FIXME to the matcher. I agree that testing if std::iterator_traits<T> exists or instantiates properly is more robust but for now excluding pointers is ok with me. According to C++ [iterator.traits] p2 the default template expects the traits to exist within the class as the matcher currently searches for.

Handling pointers as iterators will require a bit more work anyway (e.g. function pointer example you gave). I think that would be fine content for a second patch.

Since ASTMatchers will only match stuff that's actually instantiated, any suggestions on how I'd go about testing if std::iterator_traits<T> instantiates? Or perhaps we assume knowledge of how std::iterator_traits is implemented and treat all pointers and const pointers as valid iterators along with classes that have the traits members.


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