- fixed container.erase loops

diff --git a/zypp/@DOXYGEN/DOXYGEN.h b/zypp/@DOXYGEN/DOXYGEN.h
index 2b6d4ec..5acc960 100644 (file)
--- a/zypp/@DOXYGEN/DOXYGEN.h
+++ b/zypp/@DOXYGEN/DOXYGEN.h
@@ -6,7 +6,8 @@
 ////////////////////////////////////////////////////////////////////////////////
 /*! \mainpage
 
- \section coding_sec Coding style
+\section coding_sec Code snippets
+\include g_CodeSnippets
 
 */
 ////////////////////////////////////////////////////////////////////////////////

diff --git a/zypp/@DOXYGEN/g_CodeSnippets b/zypp/@DOXYGEN/g_CodeSnippets
new file mode 100644 (file)
index 0000000..0ce7789
--- /dev/null
+++ b/zypp/@DOXYGEN/g_CodeSnippets
@@ -0,0 +1,93 @@
+// Avoid buggy code, that tries to erase elements, matching a
+// certain property from containers. Example:
+//
+//  for (ResStore::iterator it = store.begin(); it != store.end(); ++it)
+//  {
+//    _pool.erase(*it);
+//  }
+//
+// Problem: Removing an element from a container invalidates (at least)
+//          all iterators pointing to it. Thus after erasing *it, it is
+//          no longer valid. ++it has UNDEFINED BEHAVIOUR.
+
+// //////////////////////////////////////////////////////////////////////
+// Loop based algorithms (differs depending on the kind of container)
+// =====================
+// //////////////////////////////////////////////////////////////////////
+
+// //////////////////////////////////////////////////////////////////////
+// Sequential container (vector string deque list): erase returns
+// a valid iterator to the next element.
+// //////////////////////////////////////////////////////////////////////
+
+  SeqContainer c;
+  for ( SeqContainer::iterator it = c.begin(); it != c.end(); /**/ )
+    {
+      if ( toBeRemoved( *it ) )
+        {
+          it = c.erase( it ); // valid next-iterator returned
+        }
+      else
+        ++it;
+    }
+
+
+// //////////////////////////////////////////////////////////////////////
+// Associative container (maps sets): erase returns void, but we can use
+// postfix increment, as ONLY iterators to the eased object get invalid:
+// //////////////////////////////////////////////////////////////////////
+
+  AssocContainer c;
+  for ( AssocContainer::iterator it = c.begin(); it != c.end(); /**/ )
+    {
+      if ( toBeRemoved( *it ) )
+        {
+          c.erase( it++ ); // postfix! Incrementing before erase
+        }
+      else
+        ++it;
+    }
+
+
+// //////////////////////////////////////////////////////////////////////
+// stl algorithms
+// ==============
+//
+// In case toBeRemoved above is actually a function/functor.
+// //////////////////////////////////////////////////////////////////////
+
+
+// //////////////////////////////////////////////////////////////////////
+// Sequential container (vector string deque): stl::remove_if,
+// does not erase elements, they are just moved to the containers
+// end, and an iterator to the 1st item to be 'removed' is returned.
+// //////////////////////////////////////////////////////////////////////
+
+  SeqContainer c;
+  c.erase( stl::remove_if( c.begin(), c.end(), toBeRemoved ),
+           c.end() );
+
+
+// //////////////////////////////////////////////////////////////////////
+// Sequential container (list): The above works too, but list has a
+// builtin remove/remove_if which is more efficient.
+// //////////////////////////////////////////////////////////////////////
+
+  list c;
+  c.remove_if( toBeRemoved );
+
+
+// //////////////////////////////////////////////////////////////////////
+// Associative container (maps sets): Actually the loop above is the most
+// efficient solution. There is an algorithm based solution, but it requires
+// copying all elements not to be removed ;(
+// //////////////////////////////////////////////////////////////////////
+
+  AssocContainer c;
+
+  AssocContainer keepItems;
+  stl::remove_copy_if( c.begin(), c.end(),
+                       stl::inserter( keepItems, keepItems.end() ),
+                       toBeRemoved );
+  c.swap( keepItems );
+

diff --git a/zypp/ResPoolManager.h b/zypp/ResPoolManager.h
index e77180e..cb04c44 100644 (file)
--- a/zypp/ResPoolManager.h
+++ b/zypp/ResPoolManager.h
@@ -64,9 +64,10 @@ namespace zypp
     void erase( ResObject::constPtr ptr_r )
     { deleter()( ptr_r ); }
 
-    /**  */
+    /** unsafe
     void erase( iterator first_r, iterator last_r )
     { std::for_each( first_r, last_r, deleter() ); }
+    */
 
     /**  */
     void clear();

diff --git a/zypp/media/MediaManager.cc b/zypp/media/MediaManager.cc
index 2aac698..fbf6d24 100644 (file)
--- a/zypp/media/MediaManager.cc
+++ b/zypp/media/MediaManager.cc
@@ -152,7 +152,7 @@ namespace zypp
           do
           {
             found = false;
-            for(it = mediaMap.begin(); it != mediaMap.end(); ++it)
+            for(it = mediaMap.begin(); it != mediaMap.end(); /**/)
             {
               if( it->second.handler->dependsOnParent())
               {
@@ -160,7 +160,9 @@ namespace zypp
                 // let it forget its parent, we will
                 // destroy it later (in clear())...
                 it->second.handler->resetParentId();
-                mediaMap.erase(it);
+                mediaMap.erase( it++ ); // postfix! Incrementing before erase
+              } else {
+                ++it;
               }
             }
           } while(found);

diff --git a/zypp/pool/PoolImpl.cc b/zypp/pool/PoolImpl.cc
index e2fab46..7069c62 100644 (file)
--- a/zypp/pool/PoolImpl.cc
+++ b/zypp/pool/PoolImpl.cc
@@ -56,11 +56,12 @@ namespace zypp
     NameHash::erase( const PoolItem & item_r )
     {
       PoolTraits::ItemContainerT items = _store[item_r->name()];
-      for (PoolTraits::iterator nit = items.begin();
-                   nit != items.end(); ++nit)
+      for ( PoolTraits::iterator nit = items.begin(); nit != items.end(); /**/ )
       {
-       if (*nit == item_r)
-           items.erase( nit );
+       if ( *nit == item_r )
+          items.erase( nit++ ); // postfix! Incrementing before erase
+        else
+          ++nit;
       }
     }
 
@@ -124,17 +125,17 @@ namespace zypp
     {
       CapSet caps = item_r->dep( cap_r );
 //XXX << "storeDelete(" << item_r << ")" << endl;
-      for (CapSet::iterator ic = caps.begin(); ic != caps.end(); ++ic) {
-       PoolTraits::CapItemContainerT capitems = store_r[cap_r][ic->index()];
-       for (PoolTraits::CapItemContainerT::iterator pos = capitems.begin();
-                                            pos != capitems.end();)
-       {
-           PoolTraits::CapItemContainerT::iterator next = pos; ++next;
-           if (pos->item == item_r)
-               capitems.erase( pos );
-           pos = next;
-       }
-      }
+      for ( CapSet::iterator ic = caps.begin(); ic != caps.end(); ++ic )
+        {
+          PoolTraits::CapItemContainerT capitems = store_r[cap_r][ic->index()];
+          for ( PoolTraits::CapItemContainerT::iterator pos = capitems.begin(); pos != capitems.end(); /**/ )
+            {
+              if ( pos->item == item_r )
+                capitems.erase( pos++ ); // postfix! Incrementing before erase
+              else
+                ++pos;
+            }
+        }
     }
 
     void CapHash::erase( const PoolItem & item_r )