extragear/multimedia/amarok/src

[Seb Ruiz]

SVN commit 593566 by seb:

Apply Ovy's atomicstring patches
I've removed building the unit test to avoid increasing build times further
CCMAIL: amarok at kde.org
CCMAIL: ovy at alum.mit.edu



 M  +35 -25    atomicstring.cpp  
 M  +15 -9     atomicstring.h  
 A             atomicstring_unittest.cpp   [License: no copyright]
 M  +4 -4      metabundle.cpp  


--- trunk/extragear/multimedia/amarok/src/atomicstring.cpp #593565:593566
@@ -98,9 +98,12 @@
 
 AtomicString::AtomicString(): m_string( 0 ) { }
 
-AtomicString::AtomicString( const AtomicString &other ): m_string( other.m_string )
+AtomicString::AtomicString( const AtomicString &other )
 {
+    storeMutex.lock();
+    m_string = other.m_string;
     ref( m_string );
+    storeMutex.unlock();
 }
 
 AtomicString::AtomicString( const QString &string ): m_string( 0 )
@@ -108,36 +111,36 @@
     if( string.isEmpty() )
         return;
 
-    Data *s = new Data( string );
+    Data *s = new Data( string );  // note: s is a shallow copy
     storeMutex.lock();
     m_string = static_cast<Data*>( *( s_store.insert( s ).first ) );
+    ref( m_string );
+    uint rc = s->refcount;
+    if( rc ) {
+	// Inserted -- we need to make s a deep copy, as this copy
+	// may be refcounted by the caller thread outside our locks
+	// Hand hold C++ as to which operator= to use
+	(QString &) (*s) = QDeepCopy<QString>(string);
+    }
     storeMutex.unlock();
-    ref( m_string );
-    if( !s->refcount )
-        delete s;
+    if ( !rc ) delete( s );	// already present
 }
 
 AtomicString::~AtomicString()
 {
+    storeMutex.lock();
     deref( m_string );
+    storeMutex.unlock();
 }
 
-QString AtomicString::string() const
-{
-    if( m_string )
-        return *m_string;
-    return QString::null;
-}
-
-const QString *AtomicString::operator->() const
-{
-    return ptr();
-}
-
 QString AtomicString::deepCopy() const
 {
-    if( m_string )
-        return QDeepCopy<QString>( *m_string );
+    if (m_string) {
+	storeMutex.lock();
+	QDeepCopy<QString> copy( *m_string );
+	storeMutex.unlock();
+	return copy;
+    }
     return QString::null;
 }
 
@@ -155,8 +158,12 @@
 
 uint AtomicString::refcount() const
 {
-    if( m_string )
-        return m_string->refcount;
+    if ( m_string ) {
+	storeMutex.lock();
+	uint rc = m_string->refcount;
+	storeMutex.unlock();
+	return rc;
+    } 
     return 0;
 }
 
@@ -164,9 +171,11 @@
 {
     if( m_string == other.m_string )
         return *this;
+    storeMutex.lock();
     deref( m_string );
     m_string = other.m_string;
     ref( m_string );
+    storeMutex.unlock();
     return *this;
 }
 
@@ -175,23 +184,24 @@
     return m_string == other.m_string;
 }
 
-void AtomicString::deref( Data *s )
+// needs to be called holding the lock
+inline void AtomicString::deref( Data *s )
 {
     if( !s )
         return;
     if( !( --s->refcount ) )
     {
-        storeMutex.lock();
         s_store.erase( s );
-        storeMutex.unlock();
         delete s;
     }
 }
 
-void AtomicString::ref( Data *s )
+// needs to be called holding the lock
+inline void AtomicString::ref( Data *s )
 {
     if( s )
         s->refcount++;
 }
 
 AtomicString::set_type AtomicString::s_store;
+QMutex AtomicString::storeMutex;
--- trunk/extragear/multimedia/amarok/src/atomicstring.h #593565:593566
@@ -85,9 +85,11 @@
     bool operator==( const AtomicString &other ) const;
 
     /**
-     * @return the string
+     * Identical to deepCopy() because of QString's thread unsafety
+     * 
+     * @return the string. 
      */
-    QString string() const;
+    inline QString string() const { return deepCopy(); }
 
     /**
      * Implicitly casts to a QString.
@@ -96,12 +98,6 @@
     inline operator QString() const { return string(); }
 
     /**
-     * For convenience, so you can do atomicstring->QStringfunction(),
-     * instead of atomicstring.string().QStringfunction().
-     */
-    const QString *operator->() const;
-
-    /**
      * Useful for threading.
      * @return a deep copy of the string
      */
@@ -126,11 +122,21 @@
      * Guaranteed to be equivalent for equivalent strings, and different for
      * different ones. This can be useful for certain kinds of hacks, but
      * shouldn't normally be used.
+     *
+     * Note: DO NOT COPY this pointer with QString() or QString=. It is not
+     * thread safe to do it (QString internal refcount)
      * @return the internal pointer to the string
      */
     const QString *ptr() const;
 
     /**
+     * For convenience, so you can do atomicstring->QStringfunction(),
+     * instead of atomicstring.string().QStringfunction(). The same warning
+     * applies as for the above ptr() function.
+     */
+    inline const QString *operator->() const { return ptr(); }
+
+    /**
      * For debugging purposes.
      * @return the number of nonempty AtomicStrings equivalent to this one
      */
@@ -160,7 +166,7 @@
 
     Data *m_string;
 
-    QMutex storeMutex;
+    static QMutex storeMutex;
 };
 
 #endif
--- trunk/extragear/multimedia/amarok/src/metabundle.cpp #593565:593566
@@ -176,7 +176,7 @@
         , m_saveFileref( 0 )
         , m_podcastBundle( 0 )
         , m_lastFmBundle( 0 )
-		, m_isSearchDirty(true)
+        , m_isSearchDirty(true)
 {
     init();
 }
@@ -210,7 +210,7 @@
     , m_saveFileref( 0 )
     , m_podcastBundle( 0 )
     , m_lastFmBundle( 0 )
-	, m_isSearchDirty(true)
+    , m_isSearchDirty(true)
 {
     if ( exists() )
     {
@@ -266,7 +266,7 @@
         , m_saveFileref( 0 )
         , m_podcastBundle( 0 )
         , m_lastFmBundle( 0 )
-		, m_isSearchDirty(true)
+	, m_isSearchDirty(true)
 {
     if( title.contains( '-' ) )
     {
@@ -862,7 +862,7 @@
 
         for (int i = 0; i < NUM_COLUMNS; i++) {
             if ((columnMask & (1 << i)) > 0) {
-                if (m_searchStr.isEmpty()) m_searchStr += ' ';
+                if (!m_searchStr.isEmpty()) m_searchStr += ' ';
                 m_searchStr += prettyText(i).lower();
             }
         }