Patchwork Fix vector C++11 allocator bug

login
register
mail settings
Submitter Jonathan Wakely
Date March 7, 2013, 10:21 p.m.
Message ID <CAH6eHdQsnsjOmzwouQy=MxHsGQdJDGhKOdBGmW5kJwVERBj5HQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/225985/
State New
Headers show

Comments

Jonathan Wakely - March 7, 2013, 10:21 p.m.
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



That fixes the forward_list failure.  I'm just testing teh vector parts now ...
Jonathan Wakely - March 7, 2013, 10:46 p.m.
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.

Patch

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());