Anyway, if we could document the limitations somewhere, this is a good
starting point to be committed.
================
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;
----------------
Edwin Vane wrote:
> 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.
> I'm not sure I understand what's wrong with the third option.
When the user is not using the standard type sugar, they are doing something
clever (or non-portable, or just weird). In my opinion, it is better to leave
such code as-is.
================
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();
----------------
Edwin Vane wrote:
> 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.
> So if vector::iterator is T*, auto wouldn't be used because int* != T*
> (unless T == int obviously).
The case I was having in mind is T==int.
My point is that the user should be using the standard type sugar at some level
(possibly adding their own sugar) in order for the transformation to be safe.
http://llvm-reviews.chandlerc.com/D392
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
