Improving strings

PolyEngine/Core/Src/pe/core/storage/IndexedString.cpp

@@ -6,12 +6,23 @@
 namespace pe::core::storage
 {
 
-IndexedString::IndexedString(const char* str)
+IndexedString::IndexedString(std::string_view str)
 : m_entry(impl::IndexedStringManager::get().registerString(str))
 {
 	ASSERTE(m_entry, "Entry is null after string creation!");
 }
 
+IndexedString::IndexedString(const impl::IndexedStringEntry* entry)
+	: m_entry(entry)
+{
+	ASSERTE(m_entry, "Entry is null after string creation!");
+}
+
+IndexedString IndexedString::FromRString(core::storage::String&& str)
+{
+	return IndexedString(impl::IndexedStringManager::get().registerString(std::move(str)));
+}
+
 IndexedString::~IndexedString()
 {
 	impl::IndexedStringManager::get().unregisterString(m_entry);

PolyEngine/Core/Src/pe/core/storage/IndexedString.hpp

@@ -22,7 +22,12 @@ class CORE_DLLEXPORT IndexedString final : public core::BaseObjectLiteralType<>
 public:
 	/// @brief IndexedString constructor. Registers the string in the IndexedStringManager.
 	/// @param[in] str String to be represented by the IndexedString instance.
-	explicit IndexedString(const char* str);
+	explicit IndexedString(std::string_view str);
+
+	/// @brief IndexedString factory function. Registers the string in the IndexedStringManager.
+	/// @note If the string is not registered yet, the memory is reused.
+	/// @param[in] str String to be represented by the IndexedString instance.
+	static IndexedString FromRString(core::storage::String&& str);
 
 	/// @brief IndexedString destructor. Unregisters the string from the IndexedStringManager.
 	~IndexedString();
@@ -59,6 +64,8 @@ class CORE_DLLEXPORT IndexedString final : public core::BaseObjectLiteralType<>
 
 	friend std::ostream& operator<< (std::ostream& stream, const IndexedString& rhs) { return stream << rhs.get(); }
 private:
+	explicit IndexedString(const impl::IndexedStringEntry* entry);
+
 	const impl::IndexedStringEntry* m_entry = nullptr;
 
 	friend struct std::hash<::pe::core::storage::IndexedString>;

PolyEngine/Core/Src/pe/core/storage/String.cpp

@@ -9,22 +9,10 @@ const String String::EMPTY = String();
 
 static const std::vector<char> WHITESPACES { ' ', '\t', '\r', '\n', '\0' };
 
-namespace pe::core::storage
-{
-
-size_t StrLen(const char* str) {
-	size_t len = 0;
-	while (str[len] != 0)
-		++len;
-	return len;
-}
-
-}
-
-String::String(const char* data) {
-	size_t length = StrLen(data);
+String::String(std::string_view str) {
+	size_t length = str.length();
 	Data.resize(length + 1);
-	std::memcpy(Data.data(), data, sizeof(char) * length);
+	std::memcpy(Data.data(), str.data(), sizeof(char) * length);
 	Data[length] = 0;
 }
 
@@ -42,8 +30,7 @@ String String::From(float var, size_t precision) { return StringBuilder().Append
 String String::From(double var) { return StringBuilder().Append(var).StealString(); }
 String String::From(double var, size_t precision) { return StringBuilder().Append(var, precision).StealString(); }
 String String::From(char var) { return StringBuilder().Append(var).StealString(); }
-String String::From(const char* var) { return StringBuilder().Append(var).StealString(); }
-String String::From(const std::string& var) { return StringBuilder().Append(var).StealString(); }
+String String::From(std::string_view var) { return StringBuilder().Append(var).StealString(); }
 
 bool String::Contains(const String& var) const
 {
@@ -202,30 +189,26 @@ String& String::operator=(String&& rhs) {
 	return *this;
 }
 
-bool String::operator==(const char* str) const {
-	if (GetLength() != StrLen(str))
+bool String::operator==(std::string_view str) const {
+	if (GetLength() != str.length())
 		return false;
 	for (size_t k = 0; k < GetLength(); ++k)
 		if (Data[k] != str[k])
 			return false;
 	return true;
 }
 
-bool String::operator==(const String& str) const {
-	return Data == str.Data;
-}
-
-bool String::operator<(const String& rhs) const {
-	if (GetLength() < rhs.GetLength())
+bool String::operator<(std::string_view rhs) const {
+	if (GetLength() < rhs.length())
 		return true;
-	else if (GetLength() > rhs.GetLength())
+	else if (GetLength() > rhs.length())
 		return false;
 
 	for (size_t i = 0; i < GetLength(); ++i)
 	{
-		if (Data[i] < rhs.Data[i])
+		if (Data[i] < rhs[i])
 			return true;
-		else if (Data[i] > rhs.Data[i])
+		else if (Data[i] > rhs[i])
 			return false;
 	}
 	return false;

PolyEngine/Core/Src/pe/core/storage/String.hpp

@@ -4,9 +4,6 @@
 
 namespace pe::core::storage
 {
-
-	CORE_DLLEXPORT size_t StrLen(const char* str);
-
 	class CORE_DLLEXPORT String final : public BaseObjectLiteralType<>
 	{
 	public:
@@ -17,17 +14,20 @@ namespace pe::core::storage
 
 		/// <summary>String constructor that creates String based on provided Cstring</summary>
 		/// <param name="data"></param>
-		String(const char* data);
+		String(const char* str) : String(std::string_view(str)) {}
+
+		/// <summary>String constructor that creates String based on provided string_view</summary>
+		/// <param name="data"></param>
+		explicit String(std::string_view str);
 
 		/// <summary>String copy constructor</summary>
 		/// <param name="rhs">Reference to String instance which state should be copied</param>
-		String(const String& rhs);
+		explicit String(const String& rhs);
 
 		/// <summary>String move constructor</summary>
 		/// <param name="rhs">Reference to String instance which state should be moved</param>
 		String(String&& rhs);
 
-
 		/// <summary>Casts int to String</summary>
 		/// <param name="var">Integer value which should be used to make String instance</param>
 		/// <returns>String containing integer value</returns>
@@ -62,15 +62,10 @@ namespace pe::core::storage
 		/// <returns>String conaining only one char</returns>
 		static String From(char var);
 
-		/// <summary>Casts Cstring to String</summary>
-		/// <param name="var">Cstring value which should be used to make String instance</param>
-		/// <returns>String containing given Cstring</returns>
-		static String From(const char* var);
-
-		/// <summary>Casts std::string to String</summary>
-		/// <param name="var">std::string reference which should be used to make String instance</param>
-		/// <returns>String containing given std::string</returns>
-		static String From(const std::string& var);
+		/// <summary>Casts string_view to String</summary>
+		/// <param name="var">string_view value which should be used to make String instance</param>
+		/// <returns>String containing given content</returns>
+		static String From(std::string_view str);
 
 
 		/// <summary>Checks if String instance contains another String instance</summary>
@@ -164,18 +159,12 @@ namespace pe::core::storage
 		/// <returns>Moved String reference</returns>
 		String& operator=(String&& rhs);
 
-		/// <summary>Compares String with Cstring</summary>
-		/// <param name="str">Cstring to be compared with</param>
-		bool operator==(const char* str) const;
-
-		/// <summary>Compares two String references</summary>
-		/// <param name="str">String to be compared with</param>
-		bool operator==(const String& str) const;
-
-		bool operator!=(const char* str) const { return !(*this == str); }
-		bool operator!=(const String& str) const { return !(*this == str); }
+		/// <summary>Compares String with string_view</summary>
+		/// <param name="str">string_view to be compared with</param>
+		bool operator==(std::string_view str) const;
+		bool operator!=(std::string_view str) const { return !(*this == str); }
 
-		bool operator<(const String& rhs) const;
+		bool operator<(std::string_view rhs) const;
 
 		/// <summary>Appends one String to another</summary>
 		/// <param name="rhs">String instance to be appended to source String</param>
@@ -197,6 +186,7 @@ namespace pe::core::storage
 
 		friend std::ostream& operator<< (std::ostream& stream, const String& rhs) { return stream << rhs.GetCStr(); }
 
+		operator std::string_view() const { return std::string_view(GetCStr()); }
 	private:
 
 		String(std::vector<char> rawData) : Data(std::move(rawData)) { Data.push_back('\0'); }

PolyEngine/Core/Src/pe/core/storage/StringBuilder.cpp

@@ -5,7 +5,7 @@
 using namespace pe::core::storage;
 
 //----------------------------------------------------------------------------
-StringBuilder& StringBuilder::Append(const char* str, const size_t length)
+StringBuilder& StringBuilder::Append(const char* str, size_t length)
 {
 	Buffer.reserve(Buffer.size() + length);
 	for (size_t i = 0; i < length; ++i)

PolyEngine/Core/Src/pe/core/storage/StringBuilder.hpp

@@ -16,17 +16,15 @@ namespace pe::core::storage
 		///	@param val Value to append
 		///	@return Instance reference for chainlinking
 		inline StringBuilder& Append(char val) { Buffer.push_back(val); return *this; }
-		inline StringBuilder& Append(const char* val) { return Append(val, strlen(val)); }
-		inline StringBuilder& Append(const std::string& val) { return Append(val.c_str(), val.length()); }
-		inline StringBuilder& Append(const String& val) { return Append(val.GetCStr(), val.GetLength()); }
 		inline StringBuilder& Append(int val) { FillBufferWithFormat(val, "%d"); return *this; }
 		inline StringBuilder& Append(long long val) { FillBufferWithFormat(val, "%lld"); return *this; }
 		inline StringBuilder& Append(unsigned val) { FillBufferWithFormat(val, "%u"); return *this; }
 		inline StringBuilder& Append(unsigned long long val) { FillBufferWithFormat(val, "%llu"); return *this; }
 		inline StringBuilder& Append(long val) { return Append((long long)val); }
 		inline StringBuilder& Append(unsigned long val) { return Append((unsigned long long)val); }
+		inline StringBuilder& Append(std::string_view str) { return Append(str.data(), str.length()); }
 
-		StringBuilder& Append(const char* str, const size_t length);
+		StringBuilder& Append(const char* str, size_t length);
 		StringBuilder& Append(f32 val, size_t precission = DEFAULT_FLT_PRECISION);
 		StringBuilder& Append(f64 val, size_t precission = DEFAULT_FLT_PRECISION);
 

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringEntry.hpp

@@ -12,6 +12,7 @@ class IndexedStringEntry final : public core::BaseObjectLiteralType<>
 {
 public:
 	IndexedStringEntry(const char* str) : m_str(str) {}
+	IndexedStringEntry(core::storage::String&& str) : m_str(std::move(str)) {}
 
 	void incrementRefCount() const { ++m_refCount; }
 	void decrementRefCount() const { --m_refCount; }

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringManager.cpp

@@ -11,32 +11,52 @@ IndexedStringManager& IndexedStringManager::get()
 	return instance;
 }
 
-const IndexedStringEntry* IndexedStringManager::registerString(const char* str)
+const IndexedStringEntry* IndexedStringManager::registerString(std::string_view str)
 {
-	IndexedStringEntry* ret = nullptr;
+	auto it = m_entries.find(str);
+	if (it == m_entries.end())
+	{
+		return shareEntry(createEntry(String(str)));
+	}
+	else
+	{
+		return shareEntry(it->second.get());
+	}
+}
 
+const IndexedStringEntry* IndexedStringManager::registerString(core::storage::String&& str)
+{
 	auto it = m_entries.find(str);
 	if (it == m_entries.end())
 	{
-		// @todo(muniu) There are two allocations happening here.
-		// Consider fixing it if the performace is affected by this.
-		auto entry = std::make_unique<IndexedStringEntry>(str);
-		ret = entry.get();
-		// We need to create a new string_view, which points to the string with the same lifetime as entry.
-		// Otherwise we could have some memory issues.
-		std::string_view strView(entry.get()->get().GetCStr());
-		m_entries.emplace(strView, std::move(entry));
+		return shareEntry(createEntry(std::move(str)));
 	}
 	else
 	{
-		ret = it->second.get();
+		return shareEntry(it->second.get());
 	}
-
-	ret->incrementRefCount();
-	ret->resetRemovalTimePoint();
+}
+
+const IndexedStringEntry* IndexedStringManager::createEntry(core::storage::String&& str)
+{
+	IndexedStringEntry* ret = nullptr;
+	auto entry = std::make_unique<IndexedStringEntry>(std::move(str));
+	ret = entry.get();
+	// We need to create a new string_view, which points to the string with the same lifetime as entry.
+	// Otherwise we could have some memory issues.
+	std::string_view strView(entry.get()->get().GetCStr());
+	m_entries.emplace(strView, std::move(entry));
+
 	return ret;
 }
 
+const IndexedStringEntry* IndexedStringManager::shareEntry(const IndexedStringEntry* entry)
+{
+	entry->incrementRefCount();
+	entry->resetRemovalTimePoint();
+	return entry;
+}
+
 void IndexedStringManager::unregisterString(const IndexedStringEntry* entry)
 {
 	entry->decrementRefCount();
@@ -75,7 +95,7 @@ void IndexedStringManager::setTTLMode(bool enabled)
 	m_ttlEnabled = enabled;
 }
 
-bool IndexedStringManager::isRegistered(const char* str) const
+bool IndexedStringManager::isRegistered(std::string_view str) const
 {
 	auto it = m_entries.find(str);
 	return it != m_entries.end();

PolyEngine/Core/Src/pe/core/storage/impl/IndexedStringManager.hpp

@@ -21,13 +21,14 @@ class CORE_DLLEXPORT IndexedStringManager final : public core::BaseObjectLiteral
 public:
 	static IndexedStringManager& get();
 
-	const IndexedStringEntry* registerString(const char* str);
+	const IndexedStringEntry* registerString(std::string_view str);
+	const IndexedStringEntry* registerString(core::storage::String&& str);
 	void unregisterString(const IndexedStringEntry* entry);
 
 	void setTTLMode(bool enabled);
 	void tickTTL(size_t ttlTickCount = 1);
 
-	bool isRegistered(const char* str) const;
+	bool isRegistered(std::string_view str) const;
 private:
 	IndexedStringManager() = default;
 
@@ -40,6 +41,9 @@ class CORE_DLLEXPORT IndexedStringManager final : public core::BaseObjectLiteral
 	void erase(const IndexedStringEntry* entry);
 	void scheduleErase(const IndexedStringEntry* entry);
 
+	const IndexedStringEntry* createEntry(core::storage::String&& str);
+	const IndexedStringEntry* shareEntry(const IndexedStringEntry* entry);
+
 	std::unordered_map<std::string_view, std::unique_ptr<IndexedStringEntry>> m_entries;
 	PriorityQueue<TTLEntry> m_ttlEntries;
 	bool m_ttlEnabled = false;