Re: Fix vector C++11 allocator bug

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 7 March 2013 22:21, Jonathan Wakely wrote:
> On 7 March 2013 21:28, François Dumont wrote:
>> Hi
>>
>>     While working on unordered containers C++11 allocator integration I used
>> forward_list tests you have done Jon. It reported some problems that should
>> have been seen on forward_list or vector allocator tests too if those tests
>> were indeed manipulating memory. But there weren't because no element was
>> ever injected in the containers.
>>
>>     So I have started injecting elements and the propagating_allocator issue
>> shown up like in my unordered container tests. The problem is that there is
>> an assertion in the allocator to check that memory is returned to the
>> correct allocator instance thanks to the personality. But when forward_list
>> or vector instances with non propagating allocators are swapped personnality
>> is not swapped and the issue. So I have introduced a bool template parameter
>> to disable all assertions regarding personality so that the allocator can
>> still be used to check that personality hasn't been exchanged.
>>
>>     Additional, making the vector tests more functional revealed a bug in
>> vector implementation.
>
> Good catch, thanks.
>
> Instead of making uneq_allocator more complicated how about just doing
> std::swap(v1, v2) again after the tests comparing get_personality?
>
> So we don't touch uneq_allocator, keep the operator== overloads in the
> test, and the diff for forward_list/allocator/swap.cc just becomes
>
> diff --git a/libstdc++-v3/testsuite/23_containers/forward_list/allocator/swap.cc
> b/libstdc++-v3/testsuite/23_containers/forward_list/allocator/swap.cc
> index a2e70b7..b5b4480 100644
> --- a/libstdc++-v3/testsuite/23_containers/forward_list/allocator/swap.cc
> +++ b/libstdc++-v3/testsuite/23_containers/forward_list/allocator/swap.cc
> @@ -48,10 +48,14 @@ void test01()
>    typedef propagating_allocator<T, false> alloc_type;
>    typedef std::forward_list<T, alloc_type> test_type;
>    test_type v1(alloc_type(1));
> +  v1.push_front(T());
>    test_type v2(alloc_type(2));
> +  v2.push_front(T());
>    std::swap(v1, v2);
>    VERIFY(1 == v1.get_allocator().get_personality());
>    VERIFY(2 == v2.get_allocator().get_personality());
> +  // swap back so assertions in uneq_allocator::deallocate don't fail
> +  std::swap(v1, v2);
>  }
>
>  void test02()
> @@ -60,7 +64,9 @@ void test02()
>    typedef propagating_allocator<T, true> alloc_type;
>    typedef std::forward_list<T, alloc_type> test_type;
>    test_type v1(alloc_type(1));
> +  v1.push_front(T());
>    test_type v2(alloc_type(2));
> +  v2.push_front(T());
>    std::swap(v1, v2);
>    VERIFY(2 == v1.get_allocator().get_personality());
>    VERIFY(1 == v2.get_allocator().get_personality());
>
>
> That fixes the forward_list failure.  I'm just testing teh vector parts now ...

As expected it works for vector/swap.cc too.  So we definitely need
the bug fix to std::vector::operator= and the testsuite changes to add
elements, but I think I'd prefer to just re-swap the containers in the
non-propagating case.