Patchwork [v3] Fix forward_list::remove vs LWG 526-type situations

login
register
mail settings
Submitter Paolo Carlini
Date Aug. 12, 2010, 12:03 a.m.
Message ID <4C633A6F.7080506@oracle.com>
Download mbox | patch
Permalink /patch/61517/
State New
Headers show

Comments

Paolo Carlini - Aug. 12, 2010, 12:03 a.m.
Hi,

noticed while adding one more use of std::__addressof to _Hashtable:
fixed exactly like the list::remove case. Tested x86_64-linux, committed.

Paolo.

////////////////////
2010-08-11  Paolo Carlini  <paolo.carlini@oracle.com>

	* include/bits/hashtable.h (_Hashtable<>::erase(const key_type&)):
	Use std::__addressof.

	* include/bits/forward_list.tcc (forward_list<>::remove): Deal
	correctly with &__tmp->_M_value == &__val.
	* testsuite/23_containers/forward_list/operations/remove_freed.cc:
	New.

Patch

Index: include/bits/hashtable.h
===================================================================
--- include/bits/hashtable.h	(revision 163100)
+++ include/bits/hashtable.h	(working copy)
@@ -1079,7 +1079,8 @@ 
 	  // _GLIBCXX_RESOLVE_LIB_DEFECTS
 	  // 526. Is it undefined if a function in the standard changes
 	  // in parameters?
-	  if (&this->_M_extract((*__slot)->_M_v) != &__k)
+	  if (std::__addressof(this->_M_extract((*__slot)->_M_v))
+	      != std::__addressof(__k))
 	    {
               _Node* __p = *__slot;
               *__slot = __p->_M_next;
Index: include/bits/forward_list.tcc
===================================================================
--- include/bits/forward_list.tcc	(revision 163100)
+++ include/bits/forward_list.tcc	(working copy)
@@ -286,13 +286,26 @@ 
     remove(const _Tp& __val)
     {
       _Node* __curr = static_cast<_Node*>(&this->_M_impl._M_head);
-      while (_Node* __temp = static_cast<_Node*>(__curr->_M_next))
+      _Node* __extra = 0;
+
+      while (_Node* __tmp = static_cast<_Node*>(__curr->_M_next))
         {
-          if (__temp->_M_value == __val)
-            this->_M_erase_after(__curr);
-          else
-            __curr = static_cast<_Node*>(__curr->_M_next);
+          if (__tmp->_M_value == __val)
+	    {
+	      if (std::__addressof(__tmp->_M_value)
+		  != std::__addressof(__val))
+		{
+		  this->_M_erase_after(__curr);
+		  continue;
+		}
+	      else
+		__extra = __curr;
+	    }
+	  __curr = static_cast<_Node*>(__curr->_M_next);
         }
+
+      if (__extra)
+	this->_M_erase_after(__extra);
     }
 
   template<typename _Tp, typename _Alloc>
@@ -302,9 +315,9 @@ 
       remove_if(_Pred __pred)
       {
 	_Node* __curr = static_cast<_Node*>(&this->_M_impl._M_head);
-        while (_Node* __temp = static_cast<_Node*>(__curr->_M_next))
+        while (_Node* __tmp = static_cast<_Node*>(__curr->_M_next))
           {
-            if (__pred(__temp->_M_value))
+            if (__pred(__tmp->_M_value))
               this->_M_erase_after(__curr);
             else
               __curr = static_cast<_Node*>(__curr->_M_next);
Index: testsuite/23_containers/forward_list/operations/remove_freed.cc
===================================================================
--- testsuite/23_containers/forward_list/operations/remove_freed.cc	(revision 0)
+++ testsuite/23_containers/forward_list/operations/remove_freed.cc	(revision 0)
@@ -0,0 +1,94 @@ 
+// { dg-options "-std=gnu++0x" }
+
+// 2010-08-11  Paolo Carlini  <paolo.carlini@oracle.com>
+//
+// Copyright (C) 2010 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <forward_list>
+#include <testsuite_hooks.h>
+
+// 23.3.3.5 forward_list operations [forwardlist.ops]
+
+// Used to cause many Valgrind errors: LWG 526-type situation.
+int test01()
+{
+  bool test __attribute__((unused)) = true;
+
+  std::forward_list<int> fl1;
+  
+  fl1.push_front(1);
+  fl1.push_front(2);
+  fl1.push_front(3);
+  fl1.push_front(4);
+  fl1.push_front(1);
+
+  fl1.remove(*fl1.begin());
+
+  VERIFY( std::distance(fl1.begin(), fl1.end()) == 3 );
+
+  auto it1 = fl1.begin();
+
+  VERIFY( *it1 == 4 );
+  ++it1;
+  VERIFY( *it1 == 3 );
+  ++it1;
+  VERIFY( *it1 == 2 );
+
+  std::forward_list<int> fl2;
+  
+  fl2.push_front(3);
+  fl2.push_front(3);
+  fl2.push_front(3);
+  fl2.push_front(3);
+  fl2.push_front(3);
+
+  auto it2 = fl2.begin();
+  ++it2;
+  ++it2;
+
+  fl2.remove(*it2);
+
+  VERIFY( std::distance(fl2.begin(), fl2.end()) == 0 );
+
+  std::forward_list<int> fl3;
+  
+  fl3.push_front(1);
+  fl3.push_front(2);
+  fl3.push_front(3);
+  fl3.push_front(3);
+  fl3.push_front(3);
+
+  auto it3 = fl3.begin();
+  ++it3;
+  ++it3;
+
+  fl3.remove(*it3);
+
+  VERIFY( std::distance(fl3.begin(), fl3.end()) == 2 );
+
+  it3 = fl3.begin();
+  VERIFY( *it3 == 2 );
+  ++it3;
+  VERIFY( *it3 == 1 );
+}
+
+int main()
+{
+  test01();
+  return 0;
+}