<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jun 7, 2017 at 2:03 PM Francis Ricci <<a href="mailto:francisjricci@gmail.com">francisjricci@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I tried to stay as close as possible to the check from std::distance,<br>
which is the function that will fail to compile if you use the<br>
templated override without an iterable type. std::distance doesn't<br>
require a forward iterator, it only requires:<br>
iterator_traits<_InputIter>::difference_type, which should be defined<br>
in the same cases as iterator_traits<_InputIter>::pointer afaik.<br></blockquote><div><br>It still seems best to not diverge from what the standard requires here.. <br><br>"<span style="font-family:DejaVuSans,"DejaVu Sans",arial,sans-serif;font-size:12.8px">This overload only participates in overload resolution if<span class="inbox-inbox-Apple-converted-space"> </span></span><code style="font-size:12.8px;font-family:DejaVuSansMono,"DejaVu Sans Mono",courier,monospace">InputIt</code><span style="font-family:DejaVuSans,"DejaVu Sans",arial,sans-serif;font-size:12.8px"><span class="inbox-inbox-Apple-converted-space"> </span>satisfies<span class="inbox-inbox-Apple-converted-space"> </span></span><a href="http://en.cppreference.com/w/cpp/concept/InputIterator" title="cpp/concept/InputIterator" style="text-decoration-line:none;color:rgb(11,0,128);background-image:none;background-position:initial;background-size:initial;background-repeat:initial;background-origin:initial;background-clip:initial;font-family:DejaVuSans,"DejaVu Sans",arial,sans-serif;font-size:12.8px"><code style="background-color:transparent;font-family:DejaVuSansMono,"DejaVu Sans Mono",courier,monospace">InputIterator</code></a><span style="font-family:DejaVuSans,"DejaVu Sans",arial,sans-serif;font-size:12.8px">." (to quote the more accessible (than the stancdard) </span><font face="DejaVuSans, DejaVu Sans, arial, sans-serif"><a href="http://en.cppreference.com/w/cpp/container/vector/assign">http://en.cppreference.com/w/cpp/container/vector/assign</a> as an example)<br></font><br>Though maybe it's impractical/not worth the hassle, I'm not sure?<br><br>I think it'd be a matter of using enable_if and checking the iterator_category from the iterator traits, though.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Wed, Jun 7, 2017 at 5:00 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> Looks a bit different from the is_forward_iterator type check that libc++<br>
> uses. Any idea what the tradeoffs are there? Why this solution is<br>
> equivalent/as good/correct?<br>
><br>
> (should have test coverage for all 3 variants that are modified)<br>
><br>
> On Wed, Jun 7, 2017 at 1:57 PM Francis Ricci via Phabricator<br>
> <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br>
>><br>
>> fjricci updated this revision to Diff 101813.<br>
>> fjricci retitled this revision from "[ADT] Ensure that correct overrides<br>
>> for SmallVector assign are selected" to "[ADT] Make iterable SmallVector<br>
>> template overrides more specific".<br>
>> fjricci edited the summary of this revision.<br>
>> fjricci added a comment.<br>
>><br>
>> Add unit test and migrate to iterator SFINAE checks<br>
>><br>
>><br>
>> <a href="https://reviews.llvm.org/D33919" rel="noreferrer" target="_blank">https://reviews.llvm.org/D33919</a><br>
>><br>
>> Files:<br>
>>   include/llvm/ADT/SmallVector.h<br>
>>   unittests/ADT/SmallVectorTest.cpp<br>
>><br>
>><br>
>> Index: unittests/ADT/SmallVectorTest.cpp<br>
>> ===================================================================<br>
>> --- unittests/ADT/SmallVectorTest.cpp<br>
>> +++ unittests/ADT/SmallVectorTest.cpp<br>
>> @@ -434,6 +434,15 @@<br>
>>    this->assertValuesInOrder(this->theVector, 3u, 1, 2, 3);<br>
>>  }<br>
>><br>
>> +// Assign test<br>
>> +TYPED_TEST(SmallVectorTest, AssignNonIterTest) {<br>
>> +  SCOPED_TRACE("AssignTest");<br>
>> +<br>
>> +  this->theVector.push_back(Constructable(1));<br>
>> +  this->theVector.assign(2, 2);<br>
>> +  this->assertValuesInOrder(this->theVector, 2u, 2);<br>
>> +}<br>
>> +<br>
>>  // Move-assign test<br>
>>  TYPED_TEST(SmallVectorTest, MoveAssignTest) {<br>
>>    SCOPED_TRACE("MoveAssignTest");<br>
>> Index: include/llvm/ADT/SmallVector.h<br>
>> ===================================================================<br>
>> --- include/llvm/ADT/SmallVector.h<br>
>> +++ include/llvm/ADT/SmallVector.h<br>
>> @@ -388,7 +388,8 @@<br>
>>    void swap(SmallVectorImpl &RHS);<br>
>><br>
>>    /// Add the specified range to the end of the SmallVector.<br>
>> -  template<typename in_iter><br>
>> +  template <typename in_iter,<br>
>> +            typename = typename std::iterator_traits<in_iter>::pointer><br>
>>    void append(in_iter in_start, in_iter in_end) {<br>
>>      size_type NumInputs = std::distance(in_start, in_end);<br>
>>      // Grow allocated space if needed.<br>
>> @@ -426,7 +427,9 @@<br>
>>      std::uninitialized_fill(this->begin(), this->end(), Elt);<br>
>>    }<br>
>><br>
>> -  template <typename in_iter> void assign(in_iter in_start, in_iter<br>
>> in_end) {<br>
>> +  template <typename in_iter,<br>
>> +            typename = typename std::iterator_traits<in_iter>::pointer><br>
>> +  void assign(in_iter in_start, in_iter in_end) {<br>
>>      clear();<br>
>>      append(in_start, in_end);<br>
>>    }<br>
>> @@ -579,7 +582,8 @@<br>
>>      return I;<br>
>>    }<br>
>><br>
>> -  template<typename ItTy><br>
>> +  template <typename ItTy,<br>
>> +            typename = typename std::iterator_traits<ItTy>::pointer><br>
>>    iterator insert(iterator I, ItTy From, ItTy To) {<br>
>>      // Convert iterator to elt# to avoid invalidating iterator when we<br>
>> reserve()<br>
>>      size_t InsertElt = I - this->begin();<br>
>> @@ -860,7 +864,8 @@<br>
>>      this->assign(Size, Value);<br>
>>    }<br>
>><br>
>> -  template<typename ItTy><br>
>> +  template <typename ItTy,<br>
>> +            typename = typename std::iterator_traits<ItTy>::pointer><br>
>>    SmallVector(ItTy S, ItTy E) : SmallVectorImpl<T>(N) {<br>
>>      this->append(S, E);<br>
>>    }<br>
>><br>
>><br>
><br>
</blockquote></div></div>