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

[Douglas Gregor]

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

> [-llvm-commits since it's not too relevant to that list at this point]
> 
> On Tue, May 15, 2012 at 11:24 AM, Douglas Gregor <dgregor at apple.com> wrote:
>> 
>> 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?
> 
> Evidently not something everyone finds to be a useful goal - and on
> that basis perhaps I should just leave it alone.
> 
> But for myself I find functions taking references (& locals that are
> references) easier to read/reason about because it's clear that
> they're not null. Trying to decide which functions are accepting
> optional values and which ones aren't makes me pause a little - though
> perhaps more out of habit than real curiosity about whether something
> is null. If switching things to references causes pointer-habitual
> people to have the same kind of pause then that's clearly not an
> outright improvement.

Unless we're seeing lots of problems due to the pointer-centric style, I don't see a motivation to make this kind of change in the code base.

>>> 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.
> 
> It is convenient, certainly. Though I find op* returning T& to be
> convenient/useful as well. In many cases we assign *i to a local
> pointer (I'm not sure why - perhaps we should just have a better name
> for the iterator and use that directly) & don't get the convenience of
> op-> (I assume this code was written before the op-> was added,
> actually) let alone the convenience of just using '.'.
> 
> But if that's what you want, I can do that.

"I.foo" vs. "I->foo" doesn't matter much to me, but "(*I)->foo" and "&*I" are really, really ugly.

>> 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>).
> 
> That's fine - though, since I want to replace
> specific/filtered_decl_iterators with some generic adapters, those
> adapters will need to provide the op-> convenience you want, whether
> or not it was specified in the concept/standard.

Yeah.

>>> 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.
> 
> For the basic decl_iterators they do (because the underlying sequence
> is Decl*) but for the specific/filtered, where it'd be SpecificDecl**
> but we don't have a real SpecificDecl* to point to, it wouldn't and
> I'd need a proxy. Are there other cases/reasons for needing a proxy
> that you're referring to?

I don't know of other cases, no.

>>> 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.
> 
> I'm fairly sure all 3 of these should be consistent, and that the
> specific/filtered should be refactored into a common adapter
> (typedef'd in place so clients of Decl wouldn't need to change, of
> course). Generally I prefer references, but I realize they're not a
> perfect fit.
> 
> Yeah - I'd like to be able to get more discussion on this & other
> stylistic (micro design?) aspects - I kind of feel like I'm just
> throwing stuff up & seeing what's stuck after a few weeks.

IRC is generally the best place for micro-design discussions.

	- Doug