* Fixed a bunch or concurreny bugs and memory leaks of the new reference

string stuff.
* It's still not thread-safe for all usage patterns, though, so we might want
  to remove or disable it: if a string is shared between several threads, and
  one of those starts to use a reference, all kinds of problems can happen.
* Some cleanup.


git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@24312 a95241bf-73f2-0310-859d-f6bbb57e9c96

headers/os/support/String.h

@@ -26,7 +26,6 @@ public:
 	const char*	String() const;
 	int32 		Length() const;
 	int32		CountChars() const;
-	int32 		ReferenceCount() const;
 
 	// Assignment
 	BString&	operator=(const BString& string);
@@ -231,7 +230,6 @@ private:
 
 	// Data
 	void		_SetLength(int32 length);
-	void		_SetReferenceCount(int32 count);
 	bool		_DoAppend(const char* string, int32 length);
 	bool		_DoPrepend(const char* string, int32 length);
 	bool		_DoInsert(const char* string, int32 offset, int32 length);
@@ -256,7 +254,11 @@ private:
 					const char* with, int32 withLen);
 
 private:
-	char*	            fPrivateData;
+	inline int32& 	_ReferenceCount();
+	bool			_IsShareable();
+	void			_FreePrivateData();
+
+	char*			fPrivateData;
 };
 
 
@@ -294,13 +296,6 @@ BString::String() const
 }
 
 
-inline int32
-BString::ReferenceCount() const
-{
-	return fPrivateData ? (*(((int32 *)fPrivateData) - 2)) : 1;
-}
-
-
 inline BString &
 BString::SetTo(const char* string)
 {
@@ -441,8 +436,7 @@ operator!=(const char *str, const BString &string)
 //	#pragma mark - BStringRef
 
 
-class BStringRef
+class BStringRef {
-{
 public:
 	BStringRef(BString& string, int32 position);
 	~BStringRef() {}
@@ -453,7 +447,7 @@ public:
 	const char* operator&() const;
 
 	BStringRef& operator=(char c);
-	BStringRef &operator=(const BStringRef &rc);
+	BStringRef& operator=(const BStringRef& rc);
 
 private:
 	BString&	fString;

src/kits/support/String.cpp

@@ -6,11 +6,11 @@
 *		Marc Flerackers ([email protected])
 *		Stefano Ceccherini ([email protected])
 *		Oliver Tappe ([email protected])
-*		Axel Dörfler, [email protected]
+*		Axel Dörfler, [email protected]
 *		Julun <[email protected]>
 */
 
-/* String class supporting common string operations. */
+/*! String class supporting common string operations. */
 
 
 #include <Debug.h>
@@ -32,6 +32,8 @@
 #define REPLACE_ALL 0x7FFFFFFF
 
 
+const uint32 kPrivateDataOffset = 2 * sizeof(int32);
+
 const char *B_EMPTY_STRING = "";
 
 
@@ -146,7 +148,7 @@ BStringRef::operator=(const BStringRef &rc)
 const char*
 BStringRef::operator&() const
 {
-	return &(fString.fPrivateData[fPosition]);
+	return &fString.fPrivateData[fPosition];
 }
 
 
@@ -154,8 +156,9 @@ char*
 BStringRef::operator&()
 {
 	fString._Detach();
-	fString._SetReferenceCount(-1);
+	fString._ReferenceCount() = -1;
-	return &(fString.fPrivateData[fPosition]);
+		// mark as unsharable
+	return &fString.fPrivateData[fPosition];
 }
 
 
@@ -180,11 +183,12 @@ BString::BString(const BString& string)
 	: fPrivateData(NULL)
 {
 	// check if source is sharable - if so, share else clone
-	if (atomic_get(&(*(((vint32*)string.fPrivateData) - 2))) >= 0) {
+	if (_IsShareable()) {
 		fPrivateData = string.fPrivateData;
-		atomic_add(&(*(((vint32*)fPrivateData) - 2)), 1);
+		atomic_add(&_ReferenceCount(), 1);
+			// string cannot go away right now
 	} else
-		fPrivateData = _Clone(string.fPrivateData, string.Length());
+		_Init(string.String(), string.Length());
 }
 
 
@@ -197,8 +201,8 @@ BString::BString(const char* string, int32 maxLength)
 
 BString::~BString()
 {
-	if (atomic_add(&(*(((vint32*)fPrivateData) - 2)), -1) < 1)
+	if (!_IsShareable() || atomic_add(&_ReferenceCount(), -1) == 1)
-		free(fPrivateData - (2 * sizeof(int32)));
+		_FreePrivateData();
 }
 
 
@@ -268,27 +272,23 @@ BString::SetTo(const BString& string)
 	if (fPrivateData == string.fPrivateData)
 		return *this;
 
-	// check if we are shared by someone else
+	bool freeData = true;
-	if (atomic_get(&(*(((vint32*)fPrivateData) - 2))) > 1) {
+
-		// we share our data with someone else
+	if (_IsShareable() && atomic_add(&_ReferenceCount(), -1) > 1) {
-		if (atomic_add(&(*((vint32*)fPrivateData - 2)), -1) < 1) {
+		// there is still someone who shares our data
-			// someone else decrement, we are the last owner
+		freeData = false;
-			free(fPrivateData - (2 * sizeof(int32)));
-		} else {
-			; // there is still someone who shares our data
-		}
-	} else {
-		// we don't share our data with someone else
-		// or we are marked as unsharable, but this will go away
-		free(fPrivateData - (2 * sizeof(int32)));
 	}
 
+	if (freeData)
+		_FreePrivateData();
+
 	// if source is sharable share, otherwise clone
-	if (atomic_get(&(*(((vint32*)string.fPrivateData) - 2))) >= 0) {
+	if (_IsShareable()) {
 		fPrivateData = string.fPrivateData;
-		atomic_add(&(*(((vint32*)fPrivateData) - 2)), 1);
+		atomic_add(&_ReferenceCount(), 1);
+			// the string cannot go away right now
 	} else
-		fPrivateData = _Clone(string.fPrivateData, string.Length());
+		_Init(string.String(), string.Length());
 
 	return *this;
 }
@@ -309,10 +309,10 @@ BString::SetTo(const BString& string, int32 maxLength)
 {
 	if (fPrivateData != string.fPrivateData
 		// make sure we reassing in case length is different
-		|| (fPrivateData == string.fPrivateData && Length() != maxLength)) {
+		|| (fPrivateData == string.fPrivateData && Length() > maxLength)) {
-			maxLength = min_clamp0(maxLength, string.Length());
+		maxLength = min_clamp0(maxLength, string.Length());
-			if (_DetachWith("", maxLength) == B_OK)
+		if (_DetachWith("", maxLength) == B_OK)
-				memcpy(fPrivateData, string.fPrivateData, maxLength);
+			memcpy(fPrivateData, string.String(), maxLength);
 	}
 	return *this;
 }
@@ -1384,7 +1384,7 @@ char&
 BString::operator[](int32 index)
 {
 	_Detach();
-	_SetReferenceCount(-1);
+	_ReferenceCount() = -1;
 
 	return fPrivateData[index];
 }
@@ -1643,24 +1643,27 @@ BString::operator<<(float f)
 //	#pragma mark - Private or reserved
 
 
+/*!	Detaches this string from an eventually shared fPrivateData.
+*/
 status_t
 BString::_Detach()
 {
 	char* newData = fPrivateData;
-	if (atomic_get(&(*(((vint32*)fPrivateData - 2)))) > 1) {
+
-		// we share our data with someone else
+	// TODO: this is not thread safe! A string could be used in than one
-		if (atomic_add(&(*(((vint32*)fPrivateData) - 2)), -1) < 1) {
+	// thread at a time (and one of those threads could create a ref)
-			; // someone else decrement, we are the last owner
+	if (atomic_get(&_ReferenceCount()) > 1) {
-		} else {
+		// It might be shared, and this requires special treatment
-			// there is still someone who shares our data
+		newData = _Clone(fPrivateData, Length());
-			newData = _Clone(fPrivateData, Length());
+		if (atomic_add(&_ReferenceCount(), -1) == 1) {
+			// someone else left, we were the last owner
+			_FreePrivateData();
 		}
-	} else {
-		; // we don't share our data with someone else
 	}
 
 	if (newData)
 		fPrivateData = newData;
+
 	return newData != NULL ? B_OK : B_NO_MEMORY;
 }
 
@@ -1671,20 +1674,18 @@ BString::_Realloc(int32 length)
 	if (length == Length())
 		return fPrivateData;
 
-	int32 offset = 2 * sizeof(int32);
+	char *dataPtr = fPrivateData ? fPrivateData - kPrivateDataOffset : NULL;
-	char *dataPtr = fPrivateData ? fPrivateData - offset : NULL;
 	if (length < 0)
 		length = 0;
 
-	dataPtr = (char*)realloc(dataPtr, length + offset + 1);
+	dataPtr = (char*)realloc(dataPtr, length + kPrivateDataOffset + 1);
 	if (dataPtr) {
-		dataPtr += offset;
+		dataPtr += kPrivateDataOffset;
 
 		fPrivateData = dataPtr;
 		fPrivateData[length] = '\0';
 
 		_SetLength(length);
-		_SetReferenceCount(1);
 	}
 	return dataPtr;
 }
@@ -1694,41 +1695,33 @@ void
 BString::_Init(const char* src, int32 length)
 {
 	fPrivateData = _Clone(src, length);
-	if (!fPrivateData) {
+	if (fPrivateData == NULL)
-		int32 offset = 2 * sizeof(int32);
+		fPrivateData = _Clone(NULL, 0);
-		fPrivateData = (char *)realloc(fPrivateData, offset + 1);
-
-		fPrivateData += offset;
-		fPrivateData[0] = '\0';
-
-		_SetLength(0);
-		_SetReferenceCount(1);
-	}
 }
 
 
 char*
 BString::_Clone(const char* data, int32 length)
 {
-	char* newData = NULL;
+	char* newData = (char *)malloc(length + kPrivateDataOffset + 1);
-	int32 offset = 2 * sizeof(int32);
+	if (newData == NULL)
-	newData = (char *)realloc(newData, length + offset + 1);
+		return NULL;
 
-	if (newData) {
+	newData += kPrivateDataOffset;
-		newData += offset;
+	newData[length] = '\0';
-		newData[length] = '\0';
 
-		*(((vint32*)newData) - 2) = 1;
+	// initialize reference count & length
-		*(((int32*)newData) - 1) = length & 0x7fffffff;
+	*(((vint32*)newData) - 2) = 1;
+	*(((int32*)newData) - 1) = length & 0x7fffffff;
+
+	if (data && length)
+		memcpy(newData, data, length);
 
-		if (data && length)
-			memcpy(newData, data, length);
-	}
 	return newData;
 }
 
 
-char *
+char*
 BString::_OpenAtBy(int32 offset, int32 length)
 {
 	int32 oldLength = Length();
@@ -1759,14 +1752,14 @@ status_t
 BString::_DetachWith(const char* string, int32 length)
 {
 	char* newData = NULL;
-	if (atomic_get(&(*(((vint32*)fPrivateData) - 2))) > 1) {
+	// TODO: this is not thread safe! A string could be used in than one
-		// we share our data with someone else
+	// thread at a time (and one of those threads could create a ref)
-		if (atomic_add(&(*(((vint32*)fPrivateData) - 2)), -1) < 1) {
+	if (atomic_get(&_ReferenceCount()) > 1) {
-			// someone else decrement, we are the last owner
+		// we might share our data with someone else
-			newData = _Realloc(length);
+		newData = _Clone(string, length);
-		} else {
+		if (atomic_add(&_ReferenceCount(), -1) == 1) {
-			// there is still someone who shares our data
+			// someone else left, we were the last owner
-			newData = _Clone(string, length);
+			_FreePrivateData();
 		}
 	} else {
 		// we don't share our data with someone else
@@ -1775,6 +1768,7 @@ BString::_DetachWith(const char* string, int32 length)
 
 	if (newData)
 		fPrivateData = newData;
+
 	return newData != NULL ? B_OK : B_NO_MEMORY;
 }
 
@@ -1786,20 +1780,34 @@ BString::_SetLength(int32 length)
 }
 
 
-void
+inline int32&
-BString::_SetReferenceCount(int32 count)
+BString::_ReferenceCount()
 {
-	*(((vint32*)fPrivateData) - 2) = count;
+	return *(((int32 *)fPrivateData) - 2);
+}
+
+
+inline bool
+BString::_IsShareable()
+{
+	return fPrivateData != NULL && _ReferenceCount() >= 0;
+}
+
+
+void
+BString::_FreePrivateData()
+{
+	free(fPrivateData - (2 * sizeof(int32)));
 }
 
 
 bool
 BString::_DoAppend(const char* string, int32 length)
 {
-	int32 len = Length();
+	int32 oldLength = Length();
-	if (_DetachWith(fPrivateData, len + length) == B_OK) {
+	if (_DetachWith(fPrivateData, oldLength + length) == B_OK) {
 		if (string && length)
-			memcpy(fPrivateData + len, string, length);
+			memcpy(fPrivateData + oldLength, string, length);
 		return true;
 	}
 	return false;
@@ -1809,9 +1817,9 @@ BString::_DoAppend(const char* string, int32 length)
 bool
 BString::_DoPrepend(const char* string, int32 length)
 {
-	int32 len = Length();
+	int32 oldLength = Length();
-	if (_DetachWith(fPrivateData, len + length) == B_OK) {
+	if (_DetachWith(fPrivateData, oldLength + length) == B_OK) {
-		memmove(fPrivateData + length, fPrivateData, len);
+		memmove(fPrivateData + length, fPrivateData, oldLength);
 		if (string && length)
 			memcpy(fPrivateData, string, length);
 		return true;
@@ -1823,9 +1831,10 @@ BString::_DoPrepend(const char* string, int32 length)
 bool
 BString::_DoInsert(const char* string, int32 offset, int32 length)
 {
-	int32 len = Length();
+	int32 oldLength = Length();
-	if (_DetachWith(fPrivateData, len + length) == B_OK) {
+	if (_DetachWith(fPrivateData, oldLength + length) == B_OK) {
-		memmove(fPrivateData + offset + length, fPrivateData + offset, len - offset);
+		memmove(fPrivateData + offset + length, fPrivateData + offset,
+			oldLength - offset);
 		if (string && length)
 			memcpy(fPrivateData + offset, string, length);
 		return true;
@@ -1903,8 +1912,8 @@ BString::_IFindBefore(const char* string, int32 offset, int32 strlen) const
 
 
 BString&
-BString::_DoCharacterEscape(const char* string,
+BString::_DoCharacterEscape(const char* string, const char *setOfCharsToEscape,
-							const char *setOfCharsToEscape, char escapeChar)
+	char escapeChar)
 {
 	if (_DetachWith(string, strlen(safestr(string))) != B_OK)
 		return *this;
@@ -1930,11 +1939,10 @@ BString::_DoCharacterEscape(const char* string,
 
 	int32 lastPos = 0;
 	char* oldAdr = fPrivateData;
-	int32 offset = 2 * sizeof(int32);
 
-	char* newData = (char*)malloc(newLength + offset + 1);
+	char* newData = (char*)malloc(newLength + kPrivateDataOffset + 1);
 	if (newData) {
-		newData += offset;
+		newData += kPrivateDataOffset;
 		char* newAdr = newData;
 		for (uint32 i = 0; i < count; ++i) {
 			pos = positions.ItemAt(i);
@@ -1952,7 +1960,7 @@ BString::_DoCharacterEscape(const char* string,
 		if (len > 0)
 			memcpy(newAdr, oldAdr, len);
 
-		free(fPrivateData - offset);
+		_FreePrivateData();
 
 		fPrivateData = newData;
 		_SetLength(newLength);
@@ -1976,7 +1984,7 @@ BString::_DoCharacterDeescape(const char* string, char escapeChar)
 
 BString&
 BString::_DoReplace(const char* findThis, const char* replaceWith,
-					int32 maxReplaceCount, int32 fromOffset, bool ignoreCase)
+	int32 maxReplaceCount, int32 fromOffset, bool ignoreCase)
 {
 	if (findThis == NULL || maxReplaceCount <= 0
 		|| fromOffset < 0 || fromOffset >= Length())
@@ -2006,7 +2014,7 @@ BString::_DoReplace(const char* findThis, const char* replaceWith,
 
 void
 BString::_ReplaceAtPositions(const PosVect* positions, int32 searchLen,
-							 const char* with, int32 withLen)
+	const char* with, int32 withLen)
 {
 	int32 len = Length();
 	uint32 count = positions->CountItems();
@@ -2020,10 +2028,9 @@ BString::_ReplaceAtPositions(const PosVect* positions, int32 searchLen,
 	int32 lastPos = 0;
 	char *oldAdr = fPrivateData;
 
-	int32 offset = 2 * sizeof(int32);
+	char *newData = (char *)malloc(newLength + kPrivateDataOffset + 1);
-	char *newData = (char *)malloc(newLength + offset + 1);
 	if (newData) {
-		newData += offset;
+		newData += kPrivateDataOffset;
 		char *newAdr = newData;
 		for (uint32 i = 0; i < count; ++i) {
 			pos = positions->ItemAt(i);
@@ -2042,7 +2049,7 @@ BString::_ReplaceAtPositions(const PosVect* positions, int32 searchLen,
 		if (len > 0)
 			memcpy(newAdr, oldAdr, len);
 
-		free(fPrivateData - offset);
+		_FreePrivateData();
 
 		fPrivateData = newData;
 		_SetLength(newLength);