[PATCH] Introducing initial UseAuto transform

Edwin Vane edwin.vane at intel.com
Tue Feb 12 08:25:46 PST 2013



================
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:
> Edwin Vane wrote:
> > 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?
> I was thinking about something like: `int *p = vector.begin()` if vector::iterator is `T*`, or `std::__internal_iterator it = ...`.  This code is already broken (or works around some issue?), and we should not make the situation worse by using auto.
> 
> Maybe I'm thinking too hard about it.
I'm not sure I'm understanding correctly but I think your concerns are handled. Auto only gets used if the type on the LHS is exactly the same as the one on the RHS, disregarding sugar. Using canonical types ensures the types really are fundamentally the same. So if vector::iterator is T*, auto wouldn't be used because int* != T* (unless T == int obviously). If a conversion exists from vector::iterator to std::__internal_iterator the code as written may compile but if those types are not the same, auto won't get used.

================
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:
> > > 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.
> Please take a look at the next page, [iterator.traits] p3.  Pointers are no more 'unusual' iterators than class types.
> 
> Although for a complete solution we should test iterator traits, we can cheat a bit and only consider functions in `std::` that are known to return iterators.  After that we can check that the type of the variable is indeed an appropriate iterator type *with sugar*.  For example, transform this:
> 
> ```std::vector<int>::iterator I = ...```
> 
> and this:
> 
> ```typedef std::vector<int>::iterator Iter;
> Iter I = ...```
> 
> but not this:
> 
> ```std::vector<int>::__Iter_type I = ...```.
> 
> > 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.
> 
> Yes, that's a valid approach, but I'm afraid that unless we check that the function is indeed returning an iterator, we will treat all functions returning pointers like they are returning iterators.
> 
> Maybe instead of hardcoding a list of known functions, we could check for 'iterator' in the type name?  (That's a weird idea indeed.)
> 
The first two cases are handled by testing the canonical types. I'm not sure I understand what's wrong with the third option.

You make a good point about handling pointers as iterators: you can't tell a pointer from a pointer used as an iterator. So definitely if we wanted to make the transform be smarter about pointers-as-iterators there would be a lot of hackery involved: checking for 'iterator' in the type name, only handling certain functions known to generate iterators (begin(), end()), etc. This could be a case for user-driven transforms: we detect that a pointer 'might' be an iterator and then we ask the user for user.

In any case, pointers-as-iterators are definitely out of scope for this patch.

================
Comment at: cpp11-migrate/UseAuto/UseAutoActions.cpp:44-47
@@ +43,6 @@
+
+    E2 = E->IgnoreConversionOperator();
+    if (E2 != E) {
+      E = E2;
+      continue;
+    }
----------------
Dmitri Gribenko wrote:
> I think this might lead to unexpected results.  Conversion operators can do nontrivial work and skipping them could create a correctness problem.
I'll remove it then and modify the logic to early-out on conversion operators.


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



More information about the cfe-commits mailing list