[llvm] r225592 - [ADT] Remove the unused default constructor for iterator_range.

[Chandler Carruth]

On Mon, Jan 12, 2015 at 11:47 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Sat, Jan 10, 2015 at 5:52 PM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>>
>> On Sat, Jan 10, 2015 at 5:16 PM, Chandler Carruth <chandlerc at gmail.com>
>> wrote:
>>
>>> Author: chandlerc
>>> Date: Sat Jan 10 19:16:26 2015
>>> New Revision: 225592
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=225592&view=rev
>>> Log:
>>> [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.


>
>
>> 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
>> way.
>>
>
> 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'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...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150113/37d00161/attachment.html>