[llvm] r225592 - [ADT] Remove the unused default constructor for iterator_range.
dblaikie at gmail.com
Tue Jan 13 08:25:30 PST 2015
On Tue, Jan 13, 2015 at 1:45 AM, Chandler Carruth <chandlerc at gmail.com>
> On Mon, Jan 12, 2015 at 11:47 AM, David Blaikie <dblaikie at gmail.com>
>> On Sat, Jan 10, 2015 at 5:52 PM, Chandler Carruth <chandlerc at gmail.com>
>>> On Sat, Jan 10, 2015 at 5:16 PM, Chandler Carruth <chandlerc at gmail.com>
>>>> Author: chandlerc
>>>> Date: Sat Jan 10 19:16:26 2015
>>>> New Revision: 225592
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=225592&view=rev
>>>> [ADT] Remove the unused default constructor for iterator_range.
>>>> This default constructor is a bit weird. It left the range in an invalid
>>>> state. That might be reasonable so that you can construct a local
>>>> iterator range and assign to it based on some logic to compute the range
>>>> you want. If folks would like to support that use case, I can add it
>>>> back, but in 238-odd usages none have actually wanted to do this. ;]
>>> Actually, there were 3 usages in Clang.
>>> We could value initialize the iterators in the default constructor, but
>>> that would preclude using iterator_range with InputIterators (restricting
>>> it to ForwardIterators and stronger). Or we could only provide the default
>>> constructor when the iterators provide a default constructor, but that's
>>> lots of template goop for what is otherwise super simple.
>> You wouldn't actually need template goop, would you - it'd just fail to
>> compile if the iterator doesn't have a default ctor. It's not like we need
>> to SFINAE it away, do we? (Well, I guess if someone ends up using
>> is_default_constructible on the range, it would fail - but I don't know
>> that it's too important to account for that until it actually comes up, but
>> maybe I'm understating the pain that would cause?)
> I wasn't sure it would work out this way, but if so, cool. Personally, I'd
> still rather add the constructor with conscious intent (and documentation)
> of forming an empty range much as value initialized forward iterators are
> required to.
Yeah, the original implementation wouldn't've worked for pointer ranges
(would need an explicit begin(T()), end(T()) in the init list, etc) so it
does seem a bit more accidental than deliberate.
>>> And I've only found 3 usages that were easily "fixed" to just pass the
>>> iterators explicitly. But I wanted to mention the option here in case
>>> others feel strongly we should make empty-range construction simple in this
>> I haven't looked closely at the clang changes, so I'm not sure - but I
>> can imagine it might be nice to easily create a null range (as we do for
>> ArrayRef quite frequently). Not too fussed, though. Maybe leaving some of
>> this as a comment in iterator_range so someone knows why it was
>> omitted/what the gotchas to adding one are if they think they need one.
>> (usually source history would suffice, but with no lines to blame, etc,
>> it's harder to do that)
> Perhaps. Honestly, I've no particularly strong feelings about this. Maybe
> if you and others come to a particular design perspective we can just
> settle with that resolution?
I don't see anyone else putting their hand up to express opinions on this,
so maybe just let it stand as-is. Though I suspect a comment might still be
in order, just to allude to the gotchas (I guess we don't usually quote
revisions in comments? I'd kind of like to be able to point to this
conversation - but I suppose the commit message can refer to this commit
and the comment will be blamed on that commit message, ec... so there's a
> I'm pretty happy to make this look however folks think is best. I went
> with the current approach because it is strictly less buggy and more
> minimal / conservative, not because I think this is definitely the right
> path forward. =]
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits