Bug 43813 - [DR1234] vector<T*>(3, NULL) fails to compile
[DR1234] vector<T*>(3, NULL) fails to compile
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: libstdc++
4.7.0
: P3 normal
: 4.8.0
Assigned To: Paolo Carlini
:
: 52522 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-20 07:06 UTC by Jeffrey Yasskin
Modified: 2012-03-07 18:44 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-04-20 09:40:35


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jeffrey Yasskin 2010-04-20 07:06:51 UTC
-------
#include <vector>
std::vector<double*> v(7, 0);
-------

lands on the

template <class InputIterator> vector(InputIterator first, InputIterator last, const Allocator& = Allocator()); 

constructor instead of

explicit vector(size_type n, const T& value = T(), const Allocator& = Allocator()); 

which the user intended. The InputIterator constructor tries to forward to _M_fill_initialize because both arguments are integral, but the "0" has now been permanently bound to the int type, stopping it from converting to a null double*.

http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#1234 indicates that the current behavior is conformant, but that the committee seems to want to require the call to do what the user intended in a future standard.

Passing "NULL" instead of "0" makes the error depend on the build target. For example, NULL appears to be an int on x86-32 but a long on x86-64, meaning the error appears on x86-32 but not on x86-64.
Comment 1 Jonathan Wakely 2010-04-20 09:40:35 UTC
(In reply to comment #0)
> Passing "NULL" instead of "0" makes the error depend on the build target. For
> example, NULL appears to be an int on x86-32 but a long on x86-64, meaning the
> error appears on x86-32 but not on x86-64.

Right, NULL is 0L on 64-bit to ensure it has the same size as (void*) and can be used as a sentinel in varargs functions such as execl

Comment 2 Jonathan Wakely 2010-04-20 09:40:55 UTC
Suspending while the issue is open
Comment 3 Paolo Carlini 2010-04-20 16:10:41 UTC
The issue has been discussed a little bit in Pittsburgh and frankly people (besides Matt) didn't show a huge interest. I think it's well possible that will not be resolved in time for C++1x (unless, as usual, one or more National Bodies submit specific comments vs the FCD about it)
Comment 4 Jeffrey Yasskin 2010-04-20 16:34:39 UTC
It seems like a QoI issue until the C++ issue is resolved, since the expected behavior is also allowed by the standard.
Comment 5 Jonathan Wakely 2010-04-20 16:50:08 UTC
patches welcome :-)
Comment 6 Jonathan Wakely 2010-04-20 16:59:53 UTC
one problem is that it's not trivial to determine if a type "qualifies as an input iterator" because a non-iterator type could provide private members which perform some of the operations supported by an iterator (e.g. operator++) and attempting to test for those members will give an access violation

there's some related discussion starting at Bug 40497 comment 23
Comment 7 Jeffrey Yasskin 2010-04-20 20:04:13 UTC
Patch request acknowledged. :) My plan if I do get around to writing it is to use enable_if<!is_integral<InputIterator>> since that's the rule [lib.sequence.reqmts]p9 asks for.
Comment 8 Paolo Carlini 2010-04-21 00:33:35 UTC
Note: if the *only* way to change the behevior for the best requires using enable_if on the user-level member functions (as, if I remember correctly, pioneered by Howard at Metrowerks quite a few years ago), we have to make sure we do it consistently over all the dispatchings in the containers, and check what happens if a binary built with the headers using the old-style dispatching is linked to a new one. In case, restrict the mechanism to C++0x mode. But really, I'd rather wait for a resolution anyway, even as NAD but clearly stating this is a QoI issue.
Comment 9 Marc Glisse 2011-10-28 09:59:58 UTC
(In reply to comment #8)
> Note: if the *only* way to change the behevior for the best requires using
> enable_if on the user-level member functions (as, if I remember correctly,
> pioneered by Howard at Metrowerks quite a few years ago),

The resolution to 1234 has been adopted for C++11. The standard now says: "the constructor shall not participate in overload resolution", not that the call should be dispatched to an appropriate function. If there is an observable difference, that makes the dispatch technique wrong.

> we have to make sure
> we do it consistently over all the dispatchings in the containers, and check
> what happens if a binary built with the headers using the old-style dispatching
> is linked to a new one. In case, restrict the mechanism to C++0x mode. But
> really, I'd rather wait for a resolution anyway, even as NAD but clearly
> stating this is a QoI issue.

I don't think adding an extra template parameter or an extra argument to the function (and removing the unnecessary functions) creates any incompatibility, but you have more experience with that.

In C++11 mode (where libstdc++ has sfinae-friendly iterator_traits), we could check something like is_convertible<typename iterator_traits<_Iterator>::iterator_category, input_iterator_tag>.
Comment 10 Jeffrey Yasskin 2012-02-22 23:38:02 UTC
->NEW since the issue was adopted for C++11: http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#1234. Clearly I didn't find time to write the patch, and I'm not likely to before gcc-4.7
Comment 11 Paolo Carlini 2012-02-23 11:25:19 UTC
At this point, it will be 4.8.0, I'm afraid.
Comment 12 Paolo Carlini 2012-02-23 11:49:10 UTC
Can do this.
Comment 13 Paolo Carlini 2012-03-04 11:32:02 UTC
Thus, since we do have indeed sfinae-friendly iterator-traits, I mean to consistently deply something like the below in all the relevant places. Even better ideas? Speak now or remain silent forever ;)

Index: include/bits/stl_deque.h
===================================================================
--- include/bits/stl_deque.h    (revision 184879)
+++ include/bits/stl_deque.h    (working copy)
@@ -888,6 +888,20 @@
        *  input iterators are used, then this will do at most 2N calls to the
        *  copy constructor, and logN memory reallocations.
        */
+#ifdef __GXX_EXPERIMENTAL_CXX0X__
+      template<typename _InputIterator, typename = typename
+              enable_if<is_convertible<typename
+                        iterator_traits<_InputIterator>::iterator_category,
+                        input_iterator_tag>::value>::type>
+        deque(_InputIterator __first, _InputIterator __last,
+             const allocator_type& __a = allocator_type())
+       : _Base(__a)
+        {
+         typedef typename iterator_traits<_InputIterator>::
+           iterator_category _IterCategory;
+         _M_range_initialize(__first, __last, _IterCategory());
+       }
+#else
       template<typename _InputIterator>
         deque(_InputIterator __first, _InputIterator __last,
              const allocator_type& __a = allocator_type())
@@ -897,6 +911,7 @@
          typedef typename std::__is_integer<_InputIterator>::__type _Integral;
          _M_initialize_dispatch(__first, __last, _Integral());
        }
+#endif
 
       /**
        *  The dtor only erases the elements, and note that if the elements
Comment 14 Paolo Carlini 2012-03-04 11:39:29 UTC
Well, besides wrapping the thing in an __is_input_iterator helper.
Comment 15 Jonathan Wakely 2012-03-04 12:00:52 UTC
(In reply to comment #14)
> Well, besides wrapping the thing in an __is_input_iterator helper.

That's what I was going to suggest, possibly using an alias template:

+#ifdef __GXX_EXPERIMENTAL_CXX0X__
+  private:
+      template<typename _InIter>
+        using _RequireInputerIter = typename
+              enable_if<is_convertible<typename
+                        iterator_traits<_InputIterator>::iterator_category,
+                        input_iterator_tag>::value>::type;
+
+  public:
+      template<typename _InputIterator, typename =
+               _RequireInputIter<_InputIterator>>
+        deque(_InputIterator __first, _InputIterator __last,
+             const allocator_type& __a = allocator_type())
+       : _Base(__a)
+        {
+         typedef typename iterator_traits<_InputIterator>::
+           iterator_category _IterCategory;
+         _M_range_initialize(__first, __last, _IterCategory());
+       }
+#else
Comment 16 Paolo Carlini 2012-03-04 12:58:51 UTC
Great. Only, I guess we want the alias somewhere out of class, we are going to use it a lot, for the debug mode containers too.
Comment 17 Jonathan Wakely 2012-03-04 13:06:40 UTC
Probably better, yes.  As aliases are cheaper to instantiate than templates it shouldn't affect compilation time or binary size to repeat it in each container, but defining it only once would be easier to maintain.
Comment 18 paolo@gcc.gnu.org 2012-03-05 01:15:32 UTC
Author: paolo
Date: Mon Mar  5 01:15:28 2012
New Revision: 184911

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=184911
Log:
2012-03-04  Paolo Carlini  <paolo.carlini@oracle.com>
	    Jonathan Wakely  <jwakely.gcc@gmail.com>

	PR libstdc++/43813
	* include/bits/stl_iterator_base_types.h (_RequireInputIter): New.
	* include/ext/vstring.h (__versa_string<>::__versa_string
	(_InputIterator, _InputIterator, const _Alloc&),
	__versa_string<>::append(_InputIterator, _InputIterator),
	__versa_string<>::assign(_InputIterator, _InputIterator),
	__versa_string<>::insert(iterator, _InputIterator,
	_InputIterator), __versa_string<>::replace(iterator, iterator,
	_InputIterator, _InputIterator)): Use it.
	* include/bits/stl_list.h (list<>::list(_InputIterator,
	_InputIterator, const allocator_type&), list<>::assign(_InputIterator,
	_InputIterator), list<>::insert(iterator, _InputIterator,
	_InputIterator)): Likewise.
	* include/bits/stl_vector.h (vector<>::vector(_InputIterator,
	_InputIterator, const allocator_type&), vector<>::assign(_InputIterator,
	_InputIterator), vectort<>::insert(iterator, _InputIterator,
	_InputIterator)): Likewise.
	* include/bits/stl_deque.h (deque<>::deque(_InputIterator,
	_InputIterator, const allocator_type&), deque<>::deque(_InputIterator,
	_InputIterator), deque<>::insert(iterator, _InputIterator,
	_InputIterator)): Likewise.
	* include/bits/stl_bvector.h (vector<>::vector(_InputIterator,
	_InputIterator, const allocator_type&), vector<>::deque(_InputIterator,
	_InputIterator), vector<>::insert(iterator, _InputIterator,
	_InputIterator)): Likewise.
	* include/bits/forward_list.h (forward_list<>::forward_list
	(_InputIterator, _InputIterator, const allocator_type&),
	forward_list<>::assign(_InputIterator, _InputIterator),
	forward_list<>::insert_after(const_iterator, _InputIterator,
	_InputIterator)): Likewise.
	(forward_list<>::_M_initialize_dispatch(,, __true_type): Remove.
	(forward_list<>::_M_range_initialize): Add, adjust everywhere.
	* include/bits/forward_list.tcc: Adjust.
	* include/debug/forward_list: Adjust.
	* include/debug/vector: Likewise.
	* include/debug/deque: Likewise.
	* include/debug/list: Likewise.
	* testsuite/ext/vstring/requirements/do_the_right_thing.cc: New.
	* testsuite/23_containers/forward_list/requirements/
	do_the_right_thing.cc: Likewise.
	* testsuite/23_containers/vector/requirements/
	do_the_right_thing.cc: Likewise.
	* testsuite/23_containers/deque/requirements/
	do_the_right_thing.cc: Likewise.
	* testsuite/23_containers/list/requirements/
	do_the_right_thing.cc: Likewise.
	* testsuite/23_containers/forward_list/requirements/dr438/
	assign_neg.cc: Adjust dg-error line number.
	* testsuite/23_containers/forward_list/requirements/dr438/
	insert_neg.cc: Likewise.
	* testsuite/23_containers/forward_list/requirements/dr438/
	constructor_1_neg.cc: Likewise.
	* testsuite/23_containers/forward_list/requirements/dr438/
	constructor_2_neg.cc: Likewise.
	* testsuite/23_containers/vector/requirements/dr438/
	assign_neg.cc: Likewise.
	* testsuite/23_containers/vector/requirements/dr438/
	insert_neg.cc: Likewise.
	* testsuite/23_containers/vector/requirements/dr438/
	constructor_1_neg.cc: Likewise.
	* testsuite/23_containers/vector/requirements/dr438/
	constructor_2_neg.cc: Likewise.
	* testsuite/23_containers/deque/requirements/dr438/
	assign_neg.cc: Likewise.
	* testsuite/23_containers/deque/requirements/dr438/
	insert_neg.cc: Likewise.
	* testsuite/23_containers/deque/requirements/dr438/
	constructor_1_neg.cc: Likewise.
	* testsuite/23_containers/deque/requirements/dr438/
	constructor_2_neg.cc: Likewise.
	* testsuite/23_containers/list/requirements/dr438/
	assign_neg.cc: Likewise.
	* testsuite/23_containers/list/requirements/dr438/
	insert_neg.cc: Likewise.
	* testsuite/23_containers/list/requirements/dr438/
	constructor_1_neg.cc: Likewise.
	* testsuite/23_containers/list/requirements/dr438/
	constructor_2_neg.cc: Likewise.

Added:
    trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/do_the_right_thing.cc
    trunk/libstdc++-v3/testsuite/23_containers/forward_list/requirements/do_the_right_thing.cc
    trunk/libstdc++-v3/testsuite/23_containers/list/requirements/do_the_right_thing.cc
    trunk/libstdc++-v3/testsuite/23_containers/vector/requirements/do_the_right_thing.cc
    trunk/libstdc++-v3/testsuite/ext/vstring/requirements/do_the_right_thing.cc
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/bits/forward_list.h
    trunk/libstdc++-v3/include/bits/forward_list.tcc
    trunk/libstdc++-v3/include/bits/stl_bvector.h
    trunk/libstdc++-v3/include/bits/stl_deque.h
    trunk/libstdc++-v3/include/bits/stl_iterator_base_types.h
    trunk/libstdc++-v3/include/bits/stl_list.h
    trunk/libstdc++-v3/include/bits/stl_vector.h
    trunk/libstdc++-v3/include/debug/deque
    trunk/libstdc++-v3/include/debug/forward_list
    trunk/libstdc++-v3/include/debug/list
    trunk/libstdc++-v3/include/debug/vector
    trunk/libstdc++-v3/include/ext/vstring.h
    trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/assign_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_1_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/constructor_2_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/deque/requirements/dr438/insert_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/forward_list/requirements/dr438/assign_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/forward_list/requirements/dr438/constructor_1_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/forward_list/requirements/dr438/constructor_2_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/forward_list/requirements/dr438/insert_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/assign_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_1_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/constructor_2_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/list/requirements/dr438/insert_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/assign_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_1_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/constructor_2_neg.cc
    trunk/libstdc++-v3/testsuite/23_containers/vector/requirements/dr438/insert_neg.cc
Comment 19 Paolo Carlini 2012-03-05 01:17:39 UTC
Done for containers and vstring. I'm intentionally leaving basic_string and its exports alone.
Comment 20 Jonathan Wakely 2012-03-07 18:44:07 UTC
*** Bug 52522 has been marked as a duplicate of this bug. ***