refactor(Placeholder): encapsulate SOO memory management and fix leak…

…s; cf. #3297 #3514
pocoproject · Apr 14, 2022 · 65aa10b · 65aa10b
1 parent c032c16
commit 65aa10b
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 36 deletions.
diff --git a/Foundation/include/Poco/Any.h b/Foundation/include/Poco/Any.h
@@ -68,9 +68,20 @@ union Placeholder
 		static const unsigned int value = SizeV;
 	};
 
-	Placeholder()
+	Placeholder(): pHolder(0)
 	{
-		erase();
+		std::memset(holder, 0, sizeof(Placeholder));
+	}
+
+	~Placeholder()
+	{
+		if (!isLocal())
+			delete pHolder;
+		else
+		{
+			if (!isEmpty())
+				reinterpret_cast<PlaceholderT*>(holder)->~PlaceholderT();
+		}
 	}
 
 	void swap(Placeholder& other)
@@ -83,6 +94,7 @@ union Placeholder
 
 	void erase()
 	{
+		if (!isLocal()) delete pHolder;
 		std::memset(holder, 0, sizeof(Placeholder));
 	}
 
@@ -105,6 +117,7 @@ union Placeholder
 	template <typename T, typename V>
 	PlaceholderT* assignStack(const V& value)
 	{
+		erase();
 		new (reinterpret_cast<PlaceholderT*>(holder)) T(value);
 		setLocal(true);
 		return reinterpret_cast<PlaceholderT*>(holder);
@@ -113,6 +126,7 @@ union Placeholder
 	template <typename T, typename V>
 	PlaceholderT* assignHeap(const V& value)
 	{
+		erase();
 		pHolder = new T(value);
 		setLocal(false);
 		return pHolder;
@@ -217,13 +231,6 @@ class Any
 		/// Destructor. If Any is locally held, calls ValueHolder destructor;
 		/// otherwise, deletes the placeholder from the heap.
 	{
-		if (!empty())
-		{
-			if (_valueHolder.isLocal())
-				destruct();
-			else
-				delete content();
-		}
 	}
 
 	Any& swap(Any& other)
@@ -244,7 +251,6 @@ class Any
 			Any tmp(*this);
 			try
 			{
-				if (_valueHolder.isLocal()) destruct();
 				construct(other);
 				other = tmp;
 			}
@@ -372,11 +378,6 @@ class Any
 			_valueHolder.erase();
 	}
 
-	void destruct()
-	{
-		content()->~ValueHolder();
-	}
-
 	Placeholder<ValueHolder> _valueHolder;
 
 

diff --git a/Foundation/include/Poco/Dynamic/Var.h b/Foundation/include/Poco/Dynamic/Var.h
@@ -661,21 +661,9 @@ class Foundation_API Var
 
 	void construct(const Var& other)
 	{
+		_placeholder.erase();
 		if (!other.isEmpty())
 			other.content()->clone(&_placeholder);
-		else
-			_placeholder.erase();
-	}
-
-	void destruct()
-	{
-		if (!isEmpty())
-		{
-			if (_placeholder.isLocal())
-				content()->~VarHolder();
-			else
-				delete content();
-		}
 	}
 
 	Placeholder<VarHolder> _placeholder;
@@ -712,7 +700,6 @@ inline void Var::swap(Var& other)
 		Var tmp(*this);
 		try
 		{
-			if (_placeholder.isLocal()) destruct();
 			construct(other);
 			other = tmp;
 		}

diff --git a/Foundation/include/Poco/Dynamic/VarHolder.h b/Foundation/include/Poco/Dynamic/VarHolder.h
@@ -307,8 +307,8 @@ class Foundation_API VarHolder
 		(void)pVarHolder;
 		return new VarHolderImpl<T>(val);
 #else
-	poco_check_ptr (pVarHolder);
-	return makeSOOHolder(pVarHolder, val);
+		poco_check_ptr (pVarHolder);
+		return makeSOOHolder(pVarHolder, val);
 #endif
 	}
 

diff --git a/Foundation/src/Var.cpp b/Foundation/src/Var.cpp
@@ -60,7 +60,6 @@ Var::Var(const Var& other)
 
 Var::~Var()
 {
-	destruct();
 }
 
 
@@ -330,8 +329,6 @@ void Var::empty()
 	delete _pHolder;
 	_pHolder = 0;
 #else
-	if (_placeholder.isLocal()) this->~Var();
-	else delete content();
 	_placeholder.erase();
 #endif
 }
@@ -343,8 +340,6 @@ void Var::clear()
 	delete _pHolder;
 	_pHolder = 0;
 #else
-	if (_placeholder.isLocal()) this->~Var();
-	else delete content();
 	_placeholder.erase();
 #endif
 }