<div class="gmail_quote">On Tue, May 15, 2012 at 3:11 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb adM"><div class="h5"><br>
On May 15, 2012, at 11:57 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="cremed">dblaikie@gmail.com</a>> wrote:<br>
<br>
> [-llvm-commits since it's not too relevant to that list at this point]<br>
><br>
> On Tue, May 15, 2012 at 11:24 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com" class="cremed">dgregor@apple.com</a>> wrote:<br>
>><br>
>> On May 15, 2012, at 9:31 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="cremed">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>>> On Tue, May 15, 2012 at 9:19 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com" class="cremed">dgregor@apple.com</a>> wrote:<br>
>>>><br>
>>>> On May 6, 2012, at 7:02 PM, David Blaikie wrote:<br>
>>>><br>
>>>>> This patch adds an iterator much like boost's transform_iterator<br>
>>>>> (without some extra bells & whistles) for use in some changes I'd like<br>
>>>>> to make to Clang to correct some misimplemented iterators there.<br>
>>>>><br>
>>>>> A few gotchas that could be improved/changed depending on opinions:<br>
>>>>> * I may be playing a little loose with the return type of the functor<br>
>>>>> (see the example/motivating functor, to_pointer) - the return type<br>
>>>>> actually must be a reference, though the result_type provides the<br>
>>>>> underlying value type, not the reference type. If this is violated<br>
>>>>> Clang will emit a warning, but I could make it more robust with a<br>
>>>>> compile time assertion in the TransformIterator that the result_type<br>
>>>>> is actually a reference type, and strip that off to provide the<br>
>>>>> value_type of the TransformIterator.<br>
>>>><br>
>>>> It's much more correct for the value and reference types of the iterator type to be, e.g.,<br>
>>>><br>
>>>> typedef typename Functor::result_type reference;<br>
>>>> typedef typename remove_reference<reference>::type value_type;<br>
>>>><br>
>>>>> * I realize adding pointer-yness back onto an iterator over references<br>
>>>>> when the underlying data structure (in the Clang patch you can see<br>
>>>>> this situation) is a sequence of pointers may be a bit overkill -<br>
>>>>> though the alternatives are writing different algorithms in the two<br>
>>>>> cases where I've used TransformIterator, or having the decl_iterator<br>
>>>>> iterate over pointers (in which case I should probably change the<br>
>>>>> other iterators I've modified to iterate over pointers for symmetry).<br>
>>>><br>
>>>> diff --git include/clang/AST/DeclBase.h include/clang/AST/DeclBase.h<br>
>>>> index 6aef681..0a16ea5 100644<br>
>>>> --- include/clang/AST/DeclBase.h<br>
>>>> +++ include/clang/AST/DeclBase.h<br>
>>>> @@ -1175,17 +1175,18 @@ public:<br>
>>>> Decl *Current;<br>
>>>><br>
>>>> public:<br>
>>>> - typedef Decl* value_type;<br>
>>>> - typedef Decl* reference;<br>
>>>> - typedef Decl* pointer;<br>
>>>> + typedef Decl value_type;<br>
>>>> + typedef value_type& reference;<br>
>>>> + typedef value_type* pointer;<br>
>>>><br>
>>>> Since we tend to traffic in declaration pointers, not references, it's really beneficial to have value_type be Decl*.<br>
>>><br>
>>> Fair enough - I was hoping to reduce the amount of pointers handed<br>
>>> around, but I realize in this case especially (where the underlying<br>
>>> sequence is a sequence of pointers) it's really an awkward fit.<br>
>><br>
>> Why is reducing the number of pointers a specific goal?<br>
><br>
> Evidently not something everyone finds to be a useful goal - and on<br>
> that basis perhaps I should just leave it alone.<br>
><br>
> But for myself I find functions taking references (& locals that are<br>
> references) easier to read/reason about because it's clear that<br>
> they're not null. Trying to decide which functions are accepting<br>
> optional values and which ones aren't makes me pause a little - though<br>
> perhaps more out of habit than real curiosity about whether something<br>
> is null. If switching things to references causes pointer-habitual<br>
> people to have the same kind of pause then that's clearly not an<br>
> outright improvement.<br>
<br>
</div></div>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.</blockquote></div><br><div>Since there was some mention of wanting other perspectives here, I wanted to chime in with my two cents...</div>
<div><br></div><div>This seems like a lot of work and a disruptive change in API patterning, and I'm really not clear what problem it's trying to solve. My suspicion is that there isn't one other than a preference for references instead of pointers.</div>
<div><br></div><div>I actually don't even agree with this movement. While yes, it is nice to use simpler constructs when there is no possibility of a null pointer, references aren't strictly superior. We can't assign to them to re-point them to other entities, which is a fundamental pattern in Clang and LLVM. I don't see soft "non-null" constraints as a sufficient reason to lose this.</div>
<div><br></div><div>What's more, it is trivial to add non-null constraints to pointers. In fact, dyn_cast/cast/isa all provide such constraints. We could teach Clang about [[nonnull]] for parameters and get much more than a reference gives us:</div>
<div><br></div><div>- Optimizing based on non-null-ness</div><div>- Checking in non-optimized builds for non-null-ness</div><div>- Still able to overwrite the argument within the funciton</div>