You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@stdcxx.apache.org by fa...@apache.org on 2007/10/22 19:23:00 UTC
svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Author: faridz
Date: Mon Oct 22 10:22:59 2007
New Revision: 587164
URL: http://svn.apache.org/viewvc?rev=587164&view=rev
Log:
2007-10-22 Farid Zaripov <fa...@epam.com>
STDCXX-495
* vector.cc (_C_insert_1): Allow inserting the values from *this.
If the value_type is a fundamental type, then save the inserting value
in a temporary copy before the moving [it; end()) range toward to end.
If the value_type is not a fundamental type, then save the pointer to
the inserting value and adjust the pointer if the inserting value belongs
to the range [it; end()).
Modified:
incubator/stdcxx/branches/4.2.x/include/vector.cc
Modified: incubator/stdcxx/branches/4.2.x/include/vector.cc
URL: http://svn.apache.org/viewvc/incubator/stdcxx/branches/4.2.x/include/vector.cc?rev=587164&r1=587163&r2=587164&view=diff
==============================================================================
--- incubator/stdcxx/branches/4.2.x/include/vector.cc (original)
+++ incubator/stdcxx/branches/4.2.x/include/vector.cc Mon Oct 22 10:22:59 2007
@@ -39,6 +39,8 @@
*
**************************************************************************/
+#include <rw/_typetraits.h> // for __rw_type_traits<>
+
_RWSTD_NAMESPACE (std) {
template <class _TypeT, class _Allocator>
@@ -178,12 +180,38 @@
// and bump up end()
_C_push_back (*(_C_end - difference_type (1)));
- // move the remaining elements from the range above one slot
- // toward the end starting with the last element
- _STD::copy_backward (__it, end () - 2, __end);
-
- // overwrite the element at the given position
- *__it = __x;
+ if (__rw::__rw_type_traits<value_type>::_C_is_class) {
+ // save the pointer to allow inserting the value from self
+ const value_type* __px = &__x;
+
+ if (&*__it < __px && __px < &*__end) {
+ // adjust the pointer if the inserting value
+ // belongs the range (__it; __end)
+ ++__px;
+ } else if (&*__it == __px) {
+ // reset the pointer to 0 to avoid
+ // self assignment __x = __x
+ __px = 0;
+ }
+
+ // move the remaining elements from the range above one slot
+ // toward the end starting with the last element
+ _STD::copy_backward (__it, end () - 2, __end);
+
+ // overwrite the element at the given position
+ if (__px)
+ *__it = *__px;
+ } else {
+ // save the copy to allow inserting the value from self
+ const value_type __tmp = __x;
+
+ // move the remaining elements from the range above one slot
+ // toward the end starting with the last element
+ _STD::copy_backward (__it, end () - 2, __end);
+
+ // overwrite the element at the given position
+ *__it = __tmp;
+ }
}
else {
// construct a copy of the value to be inserted
RE: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Farid Zaripov <Fa...@epam.com>.
> -----Original Message-----
> From: Martin Sebor [mailto:sebor@roguewave.com]
> Sent: Monday, October 22, 2007 9:25 PM
> To: stdcxx-dev@incubator.apache.org
> Subject: Re: svn commit: r587164 -
> /incubator/stdcxx/branches/4.2.x/include/vector.cc
>
> faridz@apache.org wrote:
> > Author: faridz
> > Date: Mon Oct 22 10:22:59 2007
> > New Revision: 587164
> >
> > URL: http://svn.apache.org/viewvc?rev=587164&view=rev
> > Log:
> > 2007-10-22 Farid Zaripov <fa...@epam.com>
> >
> > STDCXX-495
> Is this being used anywhere else in stdcxx yet? If not, I'd
> rather hold off introducing it until 4.3 (there's a better
> way to design the traits classes and some compilers,
> including MSVC 8, have an intrinsic implementation of some of
> these critters so it would be nice to use them wherever available).
I've reverted the change and deferred the STDCXX-495 issue to 4.3
release.
Farid.
Re: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Martin Sebor <se...@roguewave.com>.
faridz@apache.org wrote:
> Author: faridz
> Date: Mon Oct 22 10:22:59 2007
> New Revision: 587164
>
> URL: http://svn.apache.org/viewvc?rev=587164&view=rev
> Log:
> 2007-10-22 Farid Zaripov <fa...@epam.com>
>
> STDCXX-495
> * vector.cc (_C_insert_1): Allow inserting the values from *this.
> If the value_type is a fundamental type, then save the inserting value
> in a temporary copy before the moving [it; end()) range toward to end.
> If the value_type is not a fundamental type, then save the pointer to
> the inserting value and adjust the pointer if the inserting value belongs
> to the range [it; end()).
>
> Modified:
> incubator/stdcxx/branches/4.2.x/include/vector.cc
>
> Modified: incubator/stdcxx/branches/4.2.x/include/vector.cc
> URL: http://svn.apache.org/viewvc/incubator/stdcxx/branches/4.2.x/include/vector.cc?rev=587164&r1=587163&r2=587164&view=diff
> ==============================================================================
> --- incubator/stdcxx/branches/4.2.x/include/vector.cc (original)
> +++ incubator/stdcxx/branches/4.2.x/include/vector.cc Mon Oct 22 10:22:59 2007
> @@ -39,6 +39,8 @@
> *
> **************************************************************************/
>
> +#include <rw/_typetraits.h> // for __rw_type_traits<>
Is this being used anywhere else in stdcxx yet? If not, I'd rather
hold off introducing it until 4.3 (there's a better way to design
the traits classes and some compilers, including MSVC 8, have an
intrinsic implementation of some of these critters so it would
be nice to use them wherever available).
> +
> _RWSTD_NAMESPACE (std) {
>
> template <class _TypeT, class _Allocator>
> @@ -178,12 +180,38 @@
> // and bump up end()
> _C_push_back (*(_C_end - difference_type (1)));
>
> - // move the remaining elements from the range above one slot
> - // toward the end starting with the last element
> - _STD::copy_backward (__it, end () - 2, __end);
> -
> - // overwrite the element at the given position
> - *__it = __x;
> + if (__rw::__rw_type_traits<value_type>::_C_is_class) {
> + // save the pointer to allow inserting the value from self
> + const value_type* __px = &__x;
> +
> + if (&*__it < __px && __px < &*__end) {
> + // adjust the pointer if the inserting value
> + // belongs the range (__it; __end)
> + ++__px;
> + } else if (&*__it == __px) {
The stdcxx formatting style calls for the closing curly brace
to be one a line of its own.
Martin
> + // reset the pointer to 0 to avoid
> + // self assignment __x = __x
> + __px = 0;
> + }
> +
> + // move the remaining elements from the range above one slot
> + // toward the end starting with the last element
> + _STD::copy_backward (__it, end () - 2, __end);
> +
> + // overwrite the element at the given position
> + if (__px)
> + *__it = *__px;
> + } else {
> + // save the copy to allow inserting the value from self
> + const value_type __tmp = __x;
> +
> + // move the remaining elements from the range above one slot
> + // toward the end starting with the last element
> + _STD::copy_backward (__it, end () - 2, __end);
> +
> + // overwrite the element at the given position
> + *__it = __tmp;
> + }
> }
> else {
> // construct a copy of the value to be inserted
>
>
RE: svn commit: r587164 -
/incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek-4 wrote:
>
>
> Travis Vitek wrote:
>>
>>
>>Farid,
>>
>>It seems that this change would cause some potentially serious problems
>>in two different cases.
>>
>> 1. _TypeT is a UDT with an implementation of operator& that returns
>>something that isn't convertible to a pointer type. [compile error]
>> 2. _TypeT is a UDT with an implementation of opreator& that returns
>>something convertible to pointer, but the returned value is not the
>>address of the object. [runtime error]
>>
>>I see no reason that either of these cases would not be legal. I think
>>you should be using the allocator_type::address() to get the physical
>>address of a given object.
>>
>>Here is a simple testcase that illustrates the compile failure that was
>>introduced with this change.
>>
>
> FWIW, it appears that this problem exists when calling other methods of
> std::vector<S>. The vector implementation uses the function templates
> std::uninitialized_copy() and std::uninitialized_fill(), both of which
> require that the iterator types 'have operator* return an object for
> which operator& is defined and returns a pointer to T' [20.4.4 p1].
>
> I think this is another bug. Martin?
>
>
The vector members should call the overloads of the uninitialized
algorithms that take a reference to Allocator as the last argument.
Travis Vitek-4 wrote:
>
>
> If so, I'll file it seperately. Here's the testcase for those who are
> interested.
>
>
Yes, please do.
Martin
--
View this message in context: http://www.nabble.com/RE%3A-svn-commit%3A-r587164----incubator-stdcxx-branches-4.2.x-include-vector.cc-tf4672892.html#a13366994
Sent from the stdcxx-dev mailing list archive at Nabble.com.
RE: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Farid Zaripov <Fa...@epam.com>.
> -----Original Message-----
> From: Travis Vitek [mailto:Travis.Vitek@roguewave.com]
> Sent: Monday, October 22, 2007 9:38 PM
> To: stdcxx-dev@incubator.apache.org
> Subject: RE: svn commit: r587164 -
> /incubator/stdcxx/branches/4.2.x/include/vector.cc
>
>
> It seems that this change would cause some potentially
> serious problems in two different cases.
>
> 1. _TypeT is a UDT with an implementation of operator& that
> returns something that isn't convertible to a pointer type.
> [compile error]
> 2. _TypeT is a UDT with an implementation of opreator& that
> returns something convertible to pointer, but the returned
> value is not the address of the object. [runtime error]
Good catch, thanks. I've updated the patch for the issue to use
allocator::address() method.
Farid.
RE: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Travis Vitek <Tr...@roguewave.com>.
Martin Sebor wrote:
>
>Travis Vitek-4 wrote:
>>
>> Martin Sebor wrote:
>>>
>>>Travis Vitek wrote:
>>>> 1. _TypeT is a UDT with an implementation of operator&
>>>> that returns something that isn't convertible to a pointer
>>>> type.
>>>>
>>>> 2. _TypeT is a UDT with an implementation of opreator&
>>>> that returns something convertible to pointer, but the
>>>> returned value is not the address of the object.
>>>>
>>>> I see no reason that either of these cases would not be legal.
>>>
>>>Except for:
>>>http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#634
>>>and:
>>>http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#580
>>>or even:
>>>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2257.html
>>>
>>>Allocators -- what a horrible mess!
>>>
>>
>> Actually, none of those actually say it isn't legal to instantiate a
>> std::vector on a user-defined type that defines unary operator&. They
>> all relate to the 'defective' allocator interface. :P
>>
>
>How so? Issue 634 points out that allocator::address(x) is required to
>return &x.
When I said
>>>>
>>>> I see no reason that either of these cases would not be legal.
>>>>
I mean that there is no restriction in the standard that says
std::vector<UDT> for a UDT with a void returning operator& is forbidden
or has any undefined behavior [note that the standard does add this
restriction to raw_storage_iterator, uninitialized_copy,
uninitialized_fill and uninitialized_fill_n]. Given that there is no
such restriction in the standard and that other implemenations handle
the case correctly, I take that as a pretty good indication that the
code is 'legal'.
The behavior may not be defined absolutely within the standard, but I
[and possibly a few C++ Standard Library users] would expect
std::vector<UDT> would behave like std::vector<int>. The user doesn't
[and shouldn't] care how we get the address of an object, provided that
their code compiles and runs with the expected behavior.
Travis
RE: svn commit: r587164 -
/incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek-4 wrote:
>
>
>
> Martin Sebor wrote:
>>
>>Travis Vitek wrote:
>>> Farid,
>>>
>>> It seems that this change would cause some potentially
>>serious problems
>>> in two different cases.
>>>
>>> 1. _TypeT is a UDT with an implementation of operator& that returns
>>> something that isn't convertible to a pointer type. [compile error]
>>> 2. _TypeT is a UDT with an implementation of opreator& that returns
>>> something convertible to pointer, but the returned value is not the
>>> address of the object. [runtime error]
>>>
>>> I see no reason that either of these cases would not be legal.
>>
>>Except for:
>>http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#634
>>and:
>>http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#580
>>or even:
>>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2257.html
>>
>>Allocators -- what a horrible mess!
>>
>
> Actually, none of those actually say it isn't legal to instantiate a
> std::vector on a user-defined type that defines unary operator&. They
> all relate to the 'defective' allocator interface. :P
>
How so? Issue 634 points out that allocator::address(x) is required to
return &x. A type that overloads operator&() to return void instead of
T* (or pointer) breaks the function. Issue 580 notes that containers
aren't currently required to use allocator::address() but they are not
forbidden, either. And if N2257 should be adopted as a solution to
issue 580, containers wouldn't even be allowed to call address()
since the function would no longer exist.
Martin
--
View this message in context: http://www.nabble.com/RE%3A-svn-commit%3A-r587164----incubator-stdcxx-branches-4.2.x-include-vector.cc-tf4672892.html#a13366381
Sent from the stdcxx-dev mailing list archive at Nabble.com.
Re: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
>
[...]
> FYI, this issue isn't isolated to the vector container. It exists for
> most of the iterator/proxy types also. Everywhere you see
> _RWSTD_OPERATOR_ARROW, and nearly everywhere you see '&*' will have some
> issue with this. I'm sure that there are other places that the problem
> happens, but those are the ones that are easily isolated.
Some of these are probably unavoidable. For instance, the code you
used in the test case for STDCXX-612:
struct S { void operator& () const {}; };
std::reverse_iterator<S*>().operator->();
can't compile because the required Effects of the function are to
return &(operator*());
This is a can of worms. We might be able to get our containers to
deal with these funky types in some cases as an extension but making
the whole library impervious to these kinds of gotchas will most
likely be more trouble than it will be worth.
Martin
RE: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Travis Vitek <Tr...@roguewave.com>.
Martin Sebor wrote:
>
>Travis Vitek wrote:
>> Farid,
>>
>> It seems that this change would cause some potentially
>serious problems
>> in two different cases.
>>
>> 1. _TypeT is a UDT with an implementation of operator& that returns
>> something that isn't convertible to a pointer type. [compile error]
>> 2. _TypeT is a UDT with an implementation of opreator& that returns
>> something convertible to pointer, but the returned value is not the
>> address of the object. [runtime error]
>>
>> I see no reason that either of these cases would not be legal.
>
>Except for:
>http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#634
>and:
>http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#580
>or even:
>http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2257.html
>
>Allocators -- what a horrible mess!
>
Actually, none of those actually say it isn't legal to instantiate a
std::vector on a user-defined type that defines unary operator&. They
all relate to the 'defective' allocator interface. :P
>> I think
>> you should be using the allocator_type::address() to get the physical
>> address of a given object.
>
>Agreed. We have implemented the proposed resolution to #634 for
>some time so we might as well be consistent with ourselves :)
>
FYI, this issue isn't isolated to the vector container. It exists for
most of the iterator/proxy types also. Everywhere you see
_RWSTD_OPERATOR_ARROW, and nearly everywhere you see '&*' will have some
issue with this. I'm sure that there are other places that the problem
happens, but those are the ones that are easily isolated.
Travis
Re: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Martin Sebor <se...@roguewave.com>.
Travis Vitek wrote:
> Farid,
>
> It seems that this change would cause some potentially serious problems
> in two different cases.
>
> 1. _TypeT is a UDT with an implementation of operator& that returns
> something that isn't convertible to a pointer type. [compile error]
> 2. _TypeT is a UDT with an implementation of opreator& that returns
> something convertible to pointer, but the returned value is not the
> address of the object. [runtime error]
>
> I see no reason that either of these cases would not be legal.
Except for:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#634
and:
http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-active.html#580
or even:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2257.html
Allocators -- what a horrible mess!
> I think
> you should be using the allocator_type::address() to get the physical
> address of a given object.
Agreed. We have implemented the proposed resolution to #634 for
some time so we might as well be consistent with ourselves :)
Martin
>
> Here is a simple testcase that illustrates the compile failure that was
> introduced with this change.
>
> #include <vector> // for vector
>
> struct S
> {
> void operator& () const {};
> };
>
> int main ()
> {
> std::vector<S> v;
> v.insert (v.begin (), S ());
>
> return 0;
> }
>
> Travis
>
>
RE: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Travis Vitek <Tr...@roguewave.com>.
Travis Vitek wrote:
>
>
>Farid,
>
>It seems that this change would cause some potentially serious problems
>in two different cases.
>
> 1. _TypeT is a UDT with an implementation of operator& that returns
>something that isn't convertible to a pointer type. [compile error]
> 2. _TypeT is a UDT with an implementation of opreator& that returns
>something convertible to pointer, but the returned value is not the
>address of the object. [runtime error]
>
>I see no reason that either of these cases would not be legal. I think
>you should be using the allocator_type::address() to get the physical
>address of a given object.
>
>Here is a simple testcase that illustrates the compile failure that was
>introduced with this change.
>
FWIW, it appears that this problem exists when calling other methods of
std::vector<S>. The vector implementation uses the function templates
std::uninitialized_copy() and std::uninitialized_fill(), both of which
require that the iterator types 'have operator* return an object for
which operator& is defined and returns a pointer to T' [20.4.4 p1].
I think this is another bug. Martin?
If so, I'll file it seperately. Here's the testcase for those who are
interested.
#include <vector> // for vector
struct S
{
void operator& () const {};
};
int main (int argc, char** argv)
{
const S s [3];
std::vector<S> v;
// this is just a compile test, it is not intended to run
v.assign (1, s [0]);
v.assign (s, s+3);
v.at (0);
v.back ();
v.begin ();
v.capacity ();
v.empty ();
v.end ();
v.front ();
v.insert (v.begin (), s [0]);
v.insert (v.begin (), s, s+3);
v.insert (v.begin (), 2, s [0]);
v.erase (v.begin ());
v.erase (v.begin (), v.end ());
v.max_size ();
v.pop_back ();
v.push_back (s[0]);
v.rbegin ();
v.rend ();
v.reserve (10);
v.resize (10);
v.size ();
std::vector<S>().swap (v);
return 0;
}
RE: svn commit: r587164 - /incubator/stdcxx/branches/4.2.x/include/vector.cc
Posted by Travis Vitek <Tr...@roguewave.com>.
Farid,
It seems that this change would cause some potentially serious problems
in two different cases.
1. _TypeT is a UDT with an implementation of operator& that returns
something that isn't convertible to a pointer type. [compile error]
2. _TypeT is a UDT with an implementation of opreator& that returns
something convertible to pointer, but the returned value is not the
address of the object. [runtime error]
I see no reason that either of these cases would not be legal. I think
you should be using the allocator_type::address() to get the physical
address of a given object.
Here is a simple testcase that illustrates the compile failure that was
introduced with this change.
#include <vector> // for vector
struct S
{
void operator& () const {};
};
int main ()
{
std::vector<S> v;
v.insert (v.begin (), S ());
return 0;
}
Travis