Detach list when setAutoDelete() is called

When calling setAutoDelete() on the implicitly shared copy of a list,
also auto-deletion of the original container was modified because
it was not detached. On the other hand, detach() was called by some
methods getting an Iterator parameter, which can lead to modification
of other implicitly shared copies but not the object is was called on.
This happens when the method is called on a not-already-detached
container, which is normally not the case because the container is
detached when the iterator is taken (e.g. calling begin()).
In such methods detach() cannot be called, and the client must
make sure that the iterator is taken after making an implicit copy.
This will NOT work:

List<int> l1 = { 1 };
auto it = l1.begin();
List<int> l2 = l1;
l1.erase(it);

This will modify both l1 and l2. The second and the third lines
must be swapped so that l1.begin() will detach l1 from l2.

taglib/toolkit/tlist.h

@@ -121,6 +121,10 @@ namespace TagLib {
 
     /*!
      * Inserts a copy of \a item before \a it.
+     *
+     * \note This method cannot detach because \a it is tied to the internal
+     * list.  Do not make an implicitly shared copy of this list between
+     * getting the iterator and calling this method!
      */
     Iterator insert(Iterator it, const T &item);
 
@@ -199,6 +203,10 @@ namespace TagLib {
 
     /*!
      * Erase the item at \a it from the list.
+     *
+     * \note This method cannot detach because \a it is tied to the internal
+     * list.  Do not make an implicitly shared copy of this list between
+     * getting the iterator and calling this method!
      */
     Iterator erase(Iterator it);
 
@@ -232,6 +240,11 @@ namespace TagLib {
      */
     void setAutoDelete(bool autoDelete);
 
+    /*!
+     * Returns true is auto-deletion is enabled.
+     */
+    bool autoDelete() const;
+
     /*!
      * Returns a reference to item \a i in the list.
      *
@@ -293,7 +306,7 @@ namespace TagLib {
     /*
      * If this List is being shared via implicit sharing, do a deep copy of the
      * data and separate from the shared members.  This should be called by all
-     * non-const subclass members.
+     * non-const subclass members without Iterator parameters.
      */
     void detach();
 

taglib/toolkit/tlist.tcc

@@ -148,7 +148,6 @@ typename List<T>::ConstIterator List<T>::cend() const
 template <class T>
 typename List<T>::Iterator List<T>::insert(Iterator it, const T &item)
 {
-  detach();
   return d->list.insert(it, item);
 }
 
@@ -270,9 +269,16 @@ const T &List<T>::back() const
 template <class T>
 void List<T>::setAutoDelete(bool autoDelete)
 {
+  detach();
   d->autoDelete = autoDelete;
 }
 
+template <class T>
+bool List<T>::autoDelete() const
+{
+  return d->autoDelete;
+}
+
 template <class T>
 T &List<T>::back()
 {

taglib/toolkit/tmap.h

@@ -164,12 +164,16 @@ namespace TagLib {
     bool contains(const Key &key) const;
 
     /*!
-     * Erase the item at \a it from the list.
+     * Erase the item at \a it from the map.
+     *
+     * \note This method cannot detach because \a it is tied to the internal
+     * map.  Do not make an implicitly shared copy of this map between
+     * getting the iterator and calling this method!
      */
     Map<Key, T> &erase(Iterator it);
 
     /*!
-     * Erase the item with \a key from the list.
+     * Erase the item with \a key from the map.
      */
     Map<Key, T> &erase(const Key &key);
 
@@ -225,9 +229,9 @@ namespace TagLib {
 
   protected:
     /*
-     * If this List is being shared via implicit sharing, do a deep copy of the
+     * If this Map is being shared via implicit sharing, do a deep copy of the
      * data and separate from the shared members.  This should be called by all
-     * non-const subclass members.
+     * non-const subclass members without Iterator parameters.
      */
     void detach();
 

taglib/toolkit/tmap.tcc

@@ -148,7 +148,6 @@ bool Map<Key, T>::contains(const Key &key) const
 template <class Key, class T>
 Map<Key, T> &Map<Key,T>::erase(Iterator it)
 {
-  detach();
   d->map.erase(it);
   return *this;
 }

tests/test_list.cpp

@@ -60,17 +60,36 @@ public:
 
   void testDetach()
   {
-    List<int> l1;
-    l1.append(1);
-    l1.append(2);
-    l1.append(3);
-    l1.append(4);
+    {
+      List<int> l1;
+      l1.append(1);
+      l1.append(2);
+      l1.append(3);
+      l1.append(4);
 
-    List<int> l2 = l1;
-    auto it = l2.find(3);
-    *it = 33;
-    CPPUNIT_ASSERT_EQUAL(3,  l1[2]);
-    CPPUNIT_ASSERT_EQUAL(33, l2[2]);
+      List<int> l2 = l1;
+      auto it = l2.find(3);
+      *it = 33;
+      CPPUNIT_ASSERT_EQUAL(3,  l1[2]);
+      CPPUNIT_ASSERT_EQUAL(33, l2[2]);
+    }
+    {
+      List<int *> l1;
+      List<int *> l2 = l1;
+      CPPUNIT_ASSERT(!l1.autoDelete());
+      CPPUNIT_ASSERT(!l2.autoDelete());
+      l2.setAutoDelete(true);
+      CPPUNIT_ASSERT(!l1.autoDelete());
+      CPPUNIT_ASSERT(l2.autoDelete());
+    }
+    {
+      List<int> l1;
+      List<int> l2 = l1;
+      l1.insert(l1.begin(), 1);
+      CPPUNIT_ASSERT(!l1.isEmpty());
+      CPPUNIT_ASSERT_EQUAL(1, l1.front());
+      CPPUNIT_ASSERT(l2.isEmpty());
+    }
   }
 
   void bracedInit()