[PATCH] D33919: [ADT] Make iterable SmallVector template overrides more specific

[Francis Ricci via llvm-commits]

I must've been tripping over a syntax bug, because input_iterator_tag
is working just fine. Will upload.

On Thu, Jun 8, 2017 at 1:32 PM, David Blaikie <dblaikie at gmail.com> wrote:
> __is_input_iterator would be a private implementation detail of libc++ - it
> won't be portable to use that.
>
> What do you mean by 'InputIterator ~= iterator_traits-input_iterator_tag' ?
>
> On Thu, Jun 8, 2017 at 10:29 AM Francis Ricci <francisjricci at gmail.com>
> wrote:
>>
>> Unfortunately, it appears that InputIterator !=
>> iterator_traits->input_iterator_tag, as that was one of my original
>> attempts. However, it looks like std::vector's implementation uses
>> std::__is_input_iterator, which appears to work so far. I'll finish
>> testing and update if it works.
>>
>> On Thu, Jun 8, 2017 at 12:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Wed, Jun 7, 2017 at 2:03 PM Francis Ricci <francisjricci at gmail.com>
>> > wrote:
>> >>
>> >> I tried to stay as close as possible to the check from std::distance,
>> >> which is the function that will fail to compile if you use the
>> >> templated override without an iterable type. std::distance doesn't
>> >> require a forward iterator, it only requires:
>> >> iterator_traits<_InputIter>::difference_type, which should be defined
>> >> in the same cases as iterator_traits<_InputIter>::pointer afaik.
>> >
>> >
>> > It still seems best to not diverge from what the standard requires
>> > here..
>> >
>> > "This overload only participates in overload resolution if InputIt
>> > satisfies
>> > InputIterator." (to quote the more accessible (than the stancdard)
>> > http://en.cppreference.com/w/cpp/container/vector/assign as an example)
>> >
>> > Though maybe it's impractical/not worth the hassle, I'm not sure?
>> >
>> > I think it'd be a matter of using enable_if and checking the
>> > iterator_category from the iterator traits, though.
>> >
>> >>
>> >>
>> >> On Wed, Jun 7, 2017 at 5:00 PM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >> > Looks a bit different from the is_forward_iterator type check that
>> >> > libc++
>> >> > uses. Any idea what the tradeoffs are there? Why this solution is
>> >> > equivalent/as good/correct?
>> >> >
>> >> > (should have test coverage for all 3 variants that are modified)
>> >> >
>> >> > On Wed, Jun 7, 2017 at 1:57 PM Francis Ricci via Phabricator
>> >> > <reviews at reviews.llvm.org> wrote:
>> >> >>
>> >> >> fjricci updated this revision to Diff 101813.
>> >> >> fjricci retitled this revision from "[ADT] Ensure that correct
>> >> >> overrides
>> >> >> for SmallVector assign are selected" to "[ADT] Make iterable
>> >> >> SmallVector
>> >> >> template overrides more specific".
>> >> >> fjricci edited the summary of this revision.
>> >> >> fjricci added a comment.
>> >> >>
>> >> >> Add unit test and migrate to iterator SFINAE checks
>> >> >>
>> >> >>
>> >> >> https://reviews.llvm.org/D33919
>> >> >>
>> >> >> Files:
>> >> >>   include/llvm/ADT/SmallVector.h
>> >> >>   unittests/ADT/SmallVectorTest.cpp
>> >> >>
>> >> >>
>> >> >> Index: unittests/ADT/SmallVectorTest.cpp
>> >> >> ===================================================================
>> >> >> --- unittests/ADT/SmallVectorTest.cpp
>> >> >> +++ unittests/ADT/SmallVectorTest.cpp
>> >> >> @@ -434,6 +434,15 @@
>> >> >>    this->assertValuesInOrder(this->theVector, 3u, 1, 2, 3);
>> >> >>  }
>> >> >>
>> >> >> +// Assign test
>> >> >> +TYPED_TEST(SmallVectorTest, AssignNonIterTest) {
>> >> >> +  SCOPED_TRACE("AssignTest");
>> >> >> +
>> >> >> +  this->theVector.push_back(Constructable(1));
>> >> >> +  this->theVector.assign(2, 2);
>> >> >> +  this->assertValuesInOrder(this->theVector, 2u, 2);
>> >> >> +}
>> >> >> +
>> >> >>  // Move-assign test
>> >> >>  TYPED_TEST(SmallVectorTest, MoveAssignTest) {
>> >> >>    SCOPED_TRACE("MoveAssignTest");
>> >> >> Index: include/llvm/ADT/SmallVector.h
>> >> >> ===================================================================
>> >> >> --- include/llvm/ADT/SmallVector.h
>> >> >> +++ include/llvm/ADT/SmallVector.h
>> >> >> @@ -388,7 +388,8 @@
>> >> >>    void swap(SmallVectorImpl &RHS);
>> >> >>
>> >> >>    /// Add the specified range to the end of the SmallVector.
>> >> >> -  template<typename in_iter>
>> >> >> +  template <typename in_iter,
>> >> >> +            typename = typename
>> >> >> std::iterator_traits<in_iter>::pointer>
>> >> >>    void append(in_iter in_start, in_iter in_end) {
>> >> >>      size_type NumInputs = std::distance(in_start, in_end);
>> >> >>      // Grow allocated space if needed.
>> >> >> @@ -426,7 +427,9 @@
>> >> >>      std::uninitialized_fill(this->begin(), this->end(), Elt);
>> >> >>    }
>> >> >>
>> >> >> -  template <typename in_iter> void assign(in_iter in_start, in_iter
>> >> >> in_end) {
>> >> >> +  template <typename in_iter,
>> >> >> +            typename = typename
>> >> >> std::iterator_traits<in_iter>::pointer>
>> >> >> +  void assign(in_iter in_start, in_iter in_end) {
>> >> >>      clear();
>> >> >>      append(in_start, in_end);
>> >> >>    }
>> >> >> @@ -579,7 +582,8 @@
>> >> >>      return I;
>> >> >>    }
>> >> >>
>> >> >> -  template<typename ItTy>
>> >> >> +  template <typename ItTy,
>> >> >> +            typename = typename
>> >> >> std::iterator_traits<ItTy>::pointer>
>> >> >>    iterator insert(iterator I, ItTy From, ItTy To) {
>> >> >>      // Convert iterator to elt# to avoid invalidating iterator when
>> >> >> we
>> >> >> reserve()
>> >> >>      size_t InsertElt = I - this->begin();
>> >> >> @@ -860,7 +864,8 @@
>> >> >>      this->assign(Size, Value);
>> >> >>    }
>> >> >>
>> >> >> -  template<typename ItTy>
>> >> >> +  template <typename ItTy,
>> >> >> +            typename = typename
>> >> >> std::iterator_traits<ItTy>::pointer>
>> >> >>    SmallVector(ItTy S, ItTy E) : SmallVectorImpl<T>(N) {
>> >> >>      this->append(S, E);
>> >> >>    }
>> >> >>
>> >> >>
>> >> >