vector (iterator,iterator) constructor doesn't deduce second arg

[ldionne]

Summary

since compiler can not deduce second parameter from first parameter.
when invoked as

std::vector<int> v({}, std::istream_iterator<int>{});

As part of the fix made the second parameter of vector(iterator, iterator) constructor independent of the first parameter.
fixing:
https://bugs.llvm.org/show_bug.cgi?id=47497

[miscco]

So the actual problem in the bug report is that the first argument cannot be used to deduce the type.

I think it is not correct to remove the type restrictions here, they are there for a reason.

That said is there any reason the constraint is placed where it is and not in the template header where I would have expected it?

[kamleshbhalui]

In D88727#2308363, @miscco wrote:

So the actual problem in the bug report is that the first argument cannot be used to deduce the type.

I think it is not correct to remove the type restrictions here, they are there for a reason.

The only restriction is that type should be input iterator and that is still in force.

That said is there any reason the constraint is placed where it is and not in the template header where I would have expected it?

That is because the declaration and definition are at different places. And even overload with forward iterator and input iterator will collide(meaning both declarations become same).

[mclow.lists]

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp

97

You should test both combinations.

test<std::vector<int> >(input_iterator<const int*>{}, {});
test<std::vector<int> >(forward_iterator<const int*>{}, {});

[kamleshbhalui]

Added test suggested by @mclow.lists .

[kamleshbhalui]

ping?

[mclow.lists]

Is this a problem unique to vector? Do the other containers have the same constructor? Do those constructors have the same issue?
Checking ... list does not seem to have this problem. deque does not either. forward_list does not either.

<deque> has the following code:

template <class _InputIter>
    deque(_InputIter __f, _InputIter __l,
          typename enable_if<__is_cpp17_input_iterator<_InputIter>::value>::type* = 0);
template <class _InputIter>
    deque(_InputIter __f, _InputIter __l, const allocator_type& __a,
          typename enable_if<__is_cpp17_input_iterator<_InputIter>::value>::type* = 0);

I don't think that making vector different from the other containers is the right approach. Then again, I'm not 100% sure why the existing code is failing - so I should look at that.

[kamleshbhalui]

In D88727#2330286, @mclow.lists wrote:

Is this a problem unique to vector? Do the other containers have the same constructor? Do those constructors have the same issue?
Checking ... list does not seem to have this problem. deque does not either. forward_list does not either.

<deque> has the following code:

template <class _InputIter>
    deque(_InputIter __f, _InputIter __l,
          typename enable_if<__is_cpp17_input_iterator<_InputIter>::value>::type* = 0);
template <class _InputIter>
    deque(_InputIter __f, _InputIter __l, const allocator_type& __a,
          typename enable_if<__is_cpp17_input_iterator<_InputIter>::value>::type* = 0);

I don't think that making vector different from the other containers is the right approach. Then again, I'm not 100% sure why the existing code is failing - so I should look at tha

Looks like because of this patch vector deviated from other container implementation.
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130916/089030.html

[mclow.lists]

In D88727#2330308, @kamleshbhalui wrote:

Looks like because of this patch vector deviated from other container implementation.
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20130916/089030.html

and if you look at https://reviews.llvm.org/D1723, Richard Smith pointed out this exact issue ... in 2013. :-(

[mclow.lists]

So the goals of this patch should be:

• Resolve the original problem,
• Decide if the problem shown in D1723 is real, and if so, make sure that still works, and
• Make all the containers do resolve this the same way.

[mclow.lists]

In D88727#2330329, @mclow.lists wrote:

• Make all the containers do resolve this the same way.

While preserving vectors optimization for forward iterators, of course.

[ldionne]

It should be possible to use the following pattern instead, I think:

template <class _InputIterator, class = enable-if-expression>
vector(_InputIterator __first, _InputIterator __last);

This may not work in C++03 mode where default template arguments are not allowed IIRC, but maybe we can have a C++03 specific workaround?

[ldionne]

Per the update in https://github.com/llvm/llvm-project/issues/46841, it seems like this was fixed somewhere between Clang 15 and Clang 16.

I'll drop this to clear our queue as part of the Github PR transition.

libcxx/include/vector

#endif
     }
     explicit vector(size_type __n);
 #if _LIBCPP_STD_VER > 11
     explicit vector(size_type __n, const allocator_type& __a);
 #endif
     vector(size_type __n, const value_type& __x);
     vector(size_type __n, const value_type& __x, const allocator_type& __a);
     template <class _InputIterator>
-        vector(_InputIterator __first,
-               typename enable_if<__is_cpp17_input_iterator  <_InputIterator>::value &&
-                                 !__is_cpp17_forward_iterator<_InputIterator>::value &&
-                                 is_constructible<
-                                    value_type,
-                                    typename iterator_traits<_InputIterator>::reference>::value,
-                                 _InputIterator>::type __last);
-    template <class _InputIterator>
-        vector(_InputIterator __first, _InputIterator __last, const allocator_type& __a,
+        vector(_InputIterator __first, _InputIterator __last, const allocator_type& __a = allocator_type(),
                typename enable_if<__is_cpp17_input_iterator  <_InputIterator>::value &&
                                  !__is_cpp17_forward_iterator<_InputIterator>::value &&
                                  is_constructible<
                                     value_type,
                                     typename iterator_traits<_InputIterator>::reference>::value>::type* = 0);
     template <class _ForwardIterator>
-        vector(_ForwardIterator __first,
-               typename enable_if<__is_cpp17_forward_iterator<_ForwardIterator>::value &&
-                                 is_constructible<
-                                    value_type,
-                                    typename iterator_traits<_ForwardIterator>::reference>::value,
-                                 _ForwardIterator>::type __last);
-    template <class _ForwardIterator>
-        vector(_ForwardIterator __first, _ForwardIterator __last, const allocator_type& __a,
+        vector(_ForwardIterator __first, _ForwardIterator __last, const allocator_type& __a = allocator_type(),
                typename enable_if<__is_cpp17_forward_iterator<_ForwardIterator>::value &&
                                  is_constructible<
                                     value_type,
                                     typename iterator_traits<_ForwardIterator>::reference>::value>::type* = 0);
 
     _LIBCPP_INLINE_VISIBILITY
     ~vector()
     {
#endif
     {
         __vallocate(__n);
         __construct_at_end(__n, __x);
     }
 }
 
 template <class _Tp, class _Allocator>
 template <class _InputIterator>
-vector<_Tp, _Allocator>::vector(_InputIterator __first,
-       typename enable_if<__is_cpp17_input_iterator  <_InputIterator>::value &&
-                         !__is_cpp17_forward_iterator<_InputIterator>::value &&
-                         is_constructible<
-                            value_type,
-                            typename iterator_traits<_InputIterator>::reference>::value,
-                          _InputIterator>::type __last)
-{
-#if _LIBCPP_DEBUG_LEVEL >= 2
-    __get_db()->__insert_c(this);
-#endif
-    for (; __first != __last; ++__first)
-        __emplace_back(*__first);
-}
-
-template <class _Tp, class _Allocator>
-template <class _InputIterator>
 vector<_Tp, _Allocator>::vector(_InputIterator __first, _InputIterator __last, const allocator_type& __a,
                                 typename enable_if<__is_cpp17_input_iterator  <_InputIterator>::value &&
                                 !__is_cpp17_forward_iterator<_InputIterator>::value &&
                                 is_constructible<
                                    value_type,
                                    typename iterator_traits<_InputIterator>::reference>::value>::type*)
     : __base(__a)
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
     for (; __first != __last; ++__first)
         __emplace_back(*__first);
 }
 
 template <class _Tp, class _Allocator>
 template <class _ForwardIterator>
-vector<_Tp, _Allocator>::vector(_ForwardIterator __first,
-                                typename enable_if<__is_cpp17_forward_iterator<_ForwardIterator>::value &&
-                                is_constructible<
-                                   value_type,
-                                   typename iterator_traits<_ForwardIterator>::reference>::value,
-                                                   _ForwardIterator>::type __last)
-{
-#if _LIBCPP_DEBUG_LEVEL >= 2
-    __get_db()->__insert_c(this);
-#endif
-    size_type __n = static_cast<size_type>(_VSTD::distance(__first, __last));
-    if (__n > 0)
-    {
-        __vallocate(__n);
-        __construct_at_end(__first, __last, __n);
-    }
-}
-
-template <class _Tp, class _Allocator>
-template <class _ForwardIterator>
 vector<_Tp, _Allocator>::vector(_ForwardIterator __first, _ForwardIterator __last, const allocator_type& __a,
                                 typename enable_if<__is_cpp17_forward_iterator<_ForwardIterator>::value &&
                                 is_constructible<
                                    value_type,
                                    typename iterator_traits<_ForwardIterator>::reference>::value>::type*)
     : __base(__a)
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2

libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp

 #include "test_allocator.h"
 #include "min_allocator.h"
 #include "asan_testing.h"
 #if TEST_STD_VER >= 11
 #include "emplace_constructible.h"
 #include "container_test_types.h"
 #endif
 
+template <class C, class Iterator>
+void test(Iterator first, Iterator last) {
+  C c(first, last);
+  LIBCPP_ASSERT(c.__invariants());
+  assert(c.size() == static_cast<std::size_t>(std::distance(first, last)));
+  LIBCPP_ASSERT(is_contiguous_container_asan_correct(c));
+  for (typename C::const_iterator i = c.cbegin(), e = c.cend(); i != e;
+       ++i, ++first)
+    assert(*i == *first);
+}
+
 template <class C, class Iterator, class A>
 void test(Iterator first, Iterator last, const A& a) {
   C c(first, last, a);
   LIBCPP_ASSERT(c.__invariants());
   assert(c.size() == static_cast<std::size_t>(std::distance(first, last)));
   LIBCPP_ASSERT(is_contiguous_container_asan_correct(c));
   for (typename C::const_iterator i = c.cbegin(), e = c.cend(); i != e;
        ++i, ++first)
#if TEST_STD_VER >= 11
     test<std::vector<int, min_allocator<int> > >(
         bidirectional_iterator<const int*>(a),
         bidirectional_iterator<const int*>(an), alloc);
     test<std::vector<int, min_allocator<int> > >(
         random_access_iterator<const int*>(a),
         random_access_iterator<const int*>(an), alloc);
     test<std::vector<int, min_allocator<int> > >(a, an, alloc);
     test<std::vector<int, implicit_conv_allocator<int> > >(a, an, nullptr);
+    test<std::vector<int> >({}, input_iterator<const int*>{});
+    test<std::vector<int> >(input_iterator<const int*>{}, {});
+    test<std::vector<int> >({}, forward_iterator<const int*>{});
+    test<std::vector<int> >(forward_iterator<const int*>{}, {});
+
+
   }
 #endif
 }
 
 void emplaceable_concept_tests() {
 #if TEST_STD_VER >= 11
   int arr1[] = {42};
   int arr2[] = {1, 101, 42};