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

[David Blaikie via llvm-commits]

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 <http://en.cppreference.com/w/cpp/concept/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);
> >>    }
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170608/7717d661/attachment.html>