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

[David Blaikie]

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.

This means killing off the op-> for these iterators though, right? Or
do you want to keep that as an excusable convenience? 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)

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

> Of course, it's fine for the reference type to be Decl* as well.

Good point.

>> At some point I'll also be looking to add a FilterIterator (& possibly
>> a specific Filtering+Transforming iterator, as I'm not sure the
>> composition of the two independently will be great for the use case I
>> have in mind) as well since that's one of the motivation for tidying
>> up these iterators - to pull out the common functionality in the
>> specific/filtered_iterators in DeclContext.
>
>
> +//===- llvm/Support/type_traits.h - Simplfied type traits -------*- C++ -*-===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===/
>
> Header needs updating.
>
> +  pointer operator->() const {
> +    return m_func(*m_iter);
> +  }
>
> 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.

Thanks,
- David