================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
