[cfe-commits] [patch] TransformIterator (motivating use case in Clang)

[Douglas Gregor]

On May 15, 2012, at 9:31 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Tue, May 15, 2012 at 9:19 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> On May 6, 2012, at 7:02 PM, David Blaikie wrote:
>> 
>>> This patch adds an iterator much like boost's transform_iterator
>>> (without some extra bells & whistles) for use in some changes I'd like
>>> to make to Clang to correct some misimplemented iterators there.
>>> 
>>> A few gotchas that could be improved/changed depending on opinions:
>>> * I may be playing a little loose with the return type of the functor
>>> (see the example/motivating functor, to_pointer) - the return type
>>> actually must be a reference, though the result_type provides the
>>> underlying value type, not the reference type. If this is violated
>>> Clang will emit a warning, but I could make it more robust with a
>>> compile time assertion in the TransformIterator that the result_type
>>> is actually a reference type, and strip that off to provide the
>>> value_type of the TransformIterator.
>> 
>> It's much more correct for the value and reference types of the iterator type to be, e.g.,
>> 
>>  typedef typename Functor::result_type reference;
>>  typedef typename remove_reference<reference>::type value_type;
>> 
>>> * I realize adding pointer-yness back onto an iterator over references
>>> when the underlying data structure (in the Clang patch you can see
>>> this situation) is a sequence of pointers may be a bit overkill -
>>> though the alternatives are writing different algorithms in the two
>>> cases where I've used TransformIterator, or having the decl_iterator
>>> iterate over pointers (in which case I should probably change the
>>> other iterators I've modified to iterate over pointers for symmetry).
>> 
>> diff --git include/clang/AST/DeclBase.h include/clang/AST/DeclBase.h
>> index 6aef681..0a16ea5 100644
>> --- include/clang/AST/DeclBase.h
>> +++ include/clang/AST/DeclBase.h
>> @@ -1175,17 +1175,18 @@ public:
>>     Decl *Current;
>> 
>>   public:
>> -    typedef Decl*                     value_type;
>> -    typedef Decl*                     reference;
>> -    typedef Decl*                     pointer;
>> +    typedef Decl                     value_type;
>> +    typedef value_type&              reference;
>> +    typedef value_type*              pointer;
>> 
>> Since we tend to traffic in declaration pointers, not references, it's really beneficial to have value_type be Decl*.
> 
> Fair enough - I was hoping to reduce the amount of pointers handed
> around, but I realize in this case especially (where the underlying
> sequence is a sequence of pointers) it's really an awkward fit.

Why is reducing the number of pointers a specific goal? We tend use pointers  (not references) for AST nodes throughout AST, Sema, and a number of other places in the compiler.

> This means killing off the op-> for these iterators though, right? Or
> do you want to keep that as an excusable convenience?

Honestly, I preferred the world back in its conveniently-inconsistent state where operator* returned a pointer and operator-> returned that same pointer, because being able to refer to Iter->getDeclName() rather than (*Iter)->getDeclName() is *really* convenient. 

The inclusion of operator-> in the iterator concepts was a mistake. Nothing in the standard library actually depends on it, no sane algorithms need it, and it's a major PITA to implement for iterator adaptors (as you've noticed <g>).

> In which case my
> change boils down to just changing the pointer typedefs to be Decl**
> in these various iterator types. (& might make my planned work,
> generalizing the filtering iterators, a bit weird - possibly providing
> an op-> to the generic filtering iterators whenever the value type is
> a pointer to match/support this oddity)

Decl** won't always work, though. You'll almost certainly need a proxy.

> Would you prefer the other iterators I changed
> (specific_decl_iterator, filtered_decl_iterator) be switched to
> pointer value_type too?

I did prefer the pointer value_type; perhaps others want to weigh in.

>> This isn't going to work; m_func returns a value_type or reference. Dealing with -> is a real pain, because in the general case you need a proxy class.
>> 
>> +// a convenient utility for a common adaptation
>> +template<typename T>
>> +struct to_pointer {
>> +  typedef T * result_type;
>> +  typedef T argument_type;
>> +  result_type &operator()(T &t) const {
>> +    ptr = &t;
>> +    return ptr;
>> +  }
>> +private:
>> +  mutable T *ptr;
>> +};
>> 
>> 'to_pointer' is a very non-LLVM-y name.
>> 
>> More generally, this particular function object seems like a bad idea. There's nothing wrong with having an iterator type whose reference type is not a true reference, which means that there is no reason to do this little dance with the mutable 'ptr'.
> 
> Yeah, a proxy would be nicer.
> 
> In any case all this transformation stuff isn't needed for this change
> anymore if we're sticking with a pointer value_type.

Right.

	- Doug