Refactor/any soo (#3564)

* refactor(Any): SOO

- encapsulate data holders
- add missing gets and ops
- eliminate g++ warnings with enable_if's
- default enable SOO

* refactor(Placeholder): encapsulate SOO memory management and fix leaks; cf. #3297 #3514

* fix(Placeholder): asan errors and add tests

cf. #3297 #3514

Foundation/include/Poco/Any.h

@@ -38,6 +38,16 @@ template <class T> class VarHolderImpl;
 }
 
 
+template <class T, std::size_t S>
+struct TypeSizeLE:
+	std::integral_constant<bool, (sizeof(T) <= S)>{};
+
+
+template <class T, std::size_t S>
+struct TypeSizeGT:
+	std::integral_constant<bool, (sizeof(T) > S)>{};
+
+
 #ifndef POCO_NO_SOO
 
 
@@ -58,14 +68,38 @@ public:
 		static const unsigned int value = SizeV;
 	};
 
-	Placeholder()
+	Placeholder(const Placeholder&) = delete;
+	Placeholder(Placeholder&&) = delete;
+	Placeholder& operator=(const Placeholder&) = delete;
+	Placeholder& operator=(Placeholder&&) = delete;
+
+	Placeholder(): pHolder(0)
 	{
-		erase();
+		std::memset(holder, 0, sizeof(Placeholder));
+	}
+
+	~Placeholder()
+	{
+		destruct(false);
+	}
+
+	void swap(Placeholder& other)
+	{
+		if (!isLocal() && !other.isLocal())
+			std::swap(pHolder, other.pHolder);
+		else
+			throw Poco::InvalidAccessException("Placeholder::swap()");
 	}
 
 	void erase()
 	{
-		std::memset(holder, 0, sizeof(Placeholder));
+		destruct(true);
+	}
+
+	bool isEmpty() const
+	{
+		char buf[POCO_SMALL_OBJECT_SIZE] = {};
+		return 0 == std::memcmp(holder, buf, POCO_SMALL_OBJECT_SIZE);
 	}
 
 	bool isLocal() const
@@ -78,6 +112,24 @@ public:
 		holder[SizeV] = local ? 1 : 0;
 	}
 
+	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);
+	}
+
+	template <typename T, typename V>
+	PlaceholderT* assignHeap(const V& value)
+	{
+		erase();
+		pHolder = new T(value);
+		setLocal(false);
+		return pHolder;
+	}
+
 	PlaceholderT* content() const
 	{
 		if (isLocal())
@@ -86,20 +138,25 @@ public:
 			return pHolder;
 	}
 
-// MSVC71,80 won't extend friendship to nested class (Any::Holder)
-#if !defined(POCO_MSVC_VERSION) || (defined(POCO_MSVC_VERSION) && (POCO_MSVC_VERSION > 80))
 private:
-#endif
-	typedef typename std::aligned_storage<SizeV + 1>::type AlignerType;
+	typedef typename std::aligned_storage<SizeV+1>::type AlignerType;
 
-	PlaceholderT* pHolder;
-	mutable char  holder[SizeV + 1];
-	AlignerType   aligner;
+	void destruct(bool clear)
+	{
+		if (!isEmpty())
+		{
+			if (!isLocal())
+				delete pHolder;
+			else
+				reinterpret_cast<PlaceholderT*>(holder)->~PlaceholderT();
 
-	friend class Any;
-	friend class Dynamic::Var;
-	friend class Dynamic::VarHolder;
-	template <class> friend class Dynamic::VarHolderImpl;
+			if (clear) std::memset(holder, 0, sizeof(Placeholder));
+		}
+	}
+
+	PlaceholderT*         pHolder;
+	mutable unsigned char holder[SizeV+1];
+	AlignerType           aligner;
 };
 
 
@@ -185,13 +242,6 @@ public:
 		/// 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)
@@ -205,14 +255,13 @@ public:
 
 		if (!_valueHolder.isLocal() && !other._valueHolder.isLocal())
 		{
-			std::swap(_valueHolder.pHolder, other._valueHolder.pHolder);
+			_valueHolder.swap(other._valueHolder);
 		}
 		else
 		{
 			Any tmp(*this);
 			try
 			{
-				if (_valueHolder.isLocal()) destruct();
 				construct(other);
 				other = tmp;
 			}
@@ -252,8 +301,7 @@ public:
 	bool empty() const
 		/// Returns true if the Any is empty.
 	{
-		char buf[POCO_SMALL_OBJECT_SIZE] = { 0 };
-		return 0 == std::memcmp(_valueHolder.holder, buf, POCO_SMALL_OBJECT_SIZE);
+		return _valueHolder.isEmpty();
 	}
 
 	const std::type_info & type() const
@@ -290,21 +338,27 @@ private:
 
 		virtual void clone(Placeholder<ValueHolder>* pPlaceholder) const
 		{
-			if ((sizeof(Holder<ValueType>) <= POCO_SMALL_OBJECT_SIZE))
-			{
-				new ((ValueHolder*) pPlaceholder->holder) Holder(_held);
-				pPlaceholder->setLocal(true);
-			}
-			else
-			{
-				pPlaceholder->pHolder = new Holder(_held);
-				pPlaceholder->setLocal(false);
-			}
+			cloneSOO(pPlaceholder, _held);
 		}
 
 		ValueType _held;
 
 	private:
+
+		template<typename VT,
+				 typename std::enable_if<TypeSizeLE<Holder<VT>, Placeholder<VT>::Size::value>::value>::type* = nullptr>
+		static void cloneSOO(Placeholder<ValueHolder>* pPlaceholder, VT& held)
+		{
+			pPlaceholder->assignStack<Holder<ValueType>, ValueType>(held);
+		}
+
+		template<typename VT,
+				 typename std::enable_if<TypeSizeGT<Holder<VT>, Placeholder<VT>::Size::value>::value>::type* = nullptr>
+		static void cloneSOO(Placeholder<ValueHolder>* pPlaceholder, VT& held)
+		{
+			pPlaceholder->assignHeap<Holder<ValueType>, ValueType>(held);
+		}
+
 		Holder & operator = (const Holder &);
 	};
 
@@ -313,19 +367,18 @@ private:
 		return _valueHolder.content();
 	}
 
-	template<typename ValueType>
+	template<typename ValueType,
+		 typename std::enable_if<TypeSizeLE<Holder<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
 	void construct(const ValueType& value)
 	{
-		if (sizeof(Holder<ValueType>) <= Placeholder<ValueType>::Size::value)
-		{
-			new (reinterpret_cast<ValueHolder*>(_valueHolder.holder)) Holder<ValueType>(value);
-			_valueHolder.setLocal(true);
-		}
-		else
-		{
-			_valueHolder.pHolder = new Holder<ValueType>(value);
-			_valueHolder.setLocal(false);
-		}
+		_valueHolder.assignStack<Holder<ValueType>, ValueType>(value);
+	}
+
+	template<typename ValueType,
+		 typename std::enable_if<TypeSizeGT<Holder<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
+	void construct(const ValueType& value)
+	{
+		_valueHolder.assignHeap<Holder<ValueType>, ValueType>(value);
 	}
 
 	void construct(const Any& other)
@@ -336,11 +389,6 @@ private:
 			_valueHolder.erase();
 	}
 
-	void destruct()
-	{
-		content()->~ValueHolder();
-	}
-
 	Placeholder<ValueHolder> _valueHolder;
 
 

Foundation/include/Poco/Config.h

@@ -74,19 +74,15 @@
 // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 // !!! NOTE: Any/Dynamic::Var SOO will NOT work reliably   !!!
 // !!! without C++11 (std::aligned_storage in particular). !!!
-// !!! Only comment this out if your compiler has support  !!!
-// !!! for std::aligned_storage.                           !!!
 // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
 //
-#ifndef POCO_ENABLE_SOO
-#define POCO_NO_SOO
-#endif
+//#define POCO_NO_SOO
 
 
 // Small object size in bytes. When assigned to Any or Var,
 // objects larger than this value will be alocated on the heap,
 // while those smaller will be placement new-ed into an
-// internal buffer.
+// internal stack-auto-allocated buffer.
 #if !defined(POCO_SMALL_OBJECT_SIZE) && !defined(POCO_NO_SOO)
 	#define POCO_SMALL_OBJECT_SIZE 32
 #endif

Foundation/include/Poco/Dynamic/Var.h

@@ -621,10 +621,10 @@ private:
 
 	void destruct()
 	{
-		if (!isEmpty()) delete content();
+		delete _pHolder;
 	}
 
-	VarHolder* _pHolder;
+	VarHolder* _pHolder = nullptr;
 
 #else
 
@@ -633,53 +633,41 @@ private:
 		return _placeholder.content();
 	}
 
+	void destruct()
+	{
+	}
+
+	template<typename ValueType,
+			 typename std::enable_if<TypeSizeLE<VarHolderImpl<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
+	void constructSOO(const ValueType& value)
+	{
+		_placeholder.assignStack<VarHolderImpl<ValueType>, ValueType>(value);
+	}
+
+	template<typename ValueType,
+			 typename std::enable_if<TypeSizeGT<VarHolderImpl<ValueType>, Placeholder<ValueType>::Size::value>::value>::type* = nullptr>
+	void constructSOO(const ValueType& value)
+	{
+		_placeholder.assignHeap<VarHolderImpl<ValueType>, ValueType>(value);
+	}
+
 	template<typename ValueType>
 	void construct(const ValueType& value)
 	{
-		if (sizeof(VarHolderImpl<ValueType>) <= Placeholder<ValueType>::Size::value)
-		{
-			new (reinterpret_cast<VarHolder*>(_placeholder.holder)) VarHolderImpl<ValueType>(value);
-			_placeholder.setLocal(true);
-		}
-		else
-		{
-			_placeholder.pHolder = new VarHolderImpl<ValueType>(value);
-			_placeholder.setLocal(false);
-		}
+		constructSOO(value);
 	}
 
 	void construct(const char* value)
 	{
 		std::string val(value);
-		if (sizeof(VarHolderImpl<std::string>) <= Placeholder<std::string>::Size::value)
-		{
-			new (reinterpret_cast<VarHolder*>(_placeholder.holder)) VarHolderImpl<std::string>(val);
-			_placeholder.setLocal(true);
-		}
-		else
-		{
-			_placeholder.pHolder = new VarHolderImpl<std::string>(val);
-			_placeholder.setLocal(false);
-		}
+		constructSOO(val);
 	}
 
 	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;
@@ -709,14 +697,13 @@ inline void Var::swap(Var& other)
 
 	if (!_placeholder.isLocal() && !other._placeholder.isLocal())
 	{
-		std::swap(_placeholder.pHolder, other._placeholder.pHolder);
+		_placeholder.swap(other._placeholder);
 	}
 	else
 	{
 		Var tmp(*this);
 		try
 		{
-			if (_placeholder.isLocal()) destruct();
 			construct(other);
 			other = tmp;
 		}

Foundation/include/Poco/Dynamic/VarHolder.h

@@ -295,31 +295,22 @@ protected:
 
 	template <typename T>
 	VarHolder* cloneHolder(Placeholder<VarHolder>* pVarHolder, const T& val) const
-		/// Instantiates value holder wrapper. If size of the wrapper is
-		/// larger than POCO_SMALL_OBJECT_SIZE, holder is instantiated on
+		/// Instantiates value holder wrapper.
+		///
+		/// Called from clone() member function of the implementation.
+		///
+		/// When the smal object optimization is enabled (POCO_NO_SOO not
+		/// defined), if size of the wrapper is larger than
+		/// POCO_SMALL_OBJECT_SIZE, holder is instantiated on
 		/// the heap, otherwise it is instantiated in-place (in the
 		/// pre-allocated buffer inside the holder).
-		///
-		/// Called from clone() member function of the implementation when
-		/// small object optimization is enabled.
 	{
 #ifdef POCO_NO_SOO
 		(void)pVarHolder;
 		return new VarHolderImpl<T>(val);
 #else
 		poco_check_ptr (pVarHolder);
-		if ((sizeof(VarHolderImpl<T>) <= Placeholder<T>::Size::value))
-		{
-			new ((VarHolder*) pVarHolder->holder) VarHolderImpl<T>(val);
-			pVarHolder->setLocal(true);
-			return (VarHolder*) pVarHolder->holder;
-		}
-		else
-		{
-			pVarHolder->pHolder = new VarHolderImpl<T>(val);
-			pVarHolder->setLocal(false);
-			return pVarHolder->pHolder;
-		}
+		return makeSOOHolder(pVarHolder, val);
 #endif
 	}
 
@@ -420,6 +411,25 @@ protected:
 	}
 
 private:
+
+#ifndef POCO_NO_SOO
+	template<typename T,
+			 typename std::enable_if<TypeSizeLE<VarHolderImpl<T>, Placeholder<T>::Size::value>::value>::type* = nullptr>
+	VarHolder* makeSOOHolder(Placeholder<VarHolder>* pVarHolder, const T& val) const
+	{
+		poco_check_ptr (pVarHolder);
+		return pVarHolder->assignStack<VarHolderImpl<T>, T>(val);
+	}
+
+	template<typename T,
+			 typename std::enable_if<TypeSizeGT<VarHolderImpl<T>, Placeholder<T>::Size::value>::value>::type* = nullptr>
+	VarHolder* makeSOOHolder(Placeholder<VarHolder>* pVarHolder, const T& val) const
+	{
+		poco_check_ptr (pVarHolder);
+		return pVarHolder->assignHeap<VarHolderImpl<T>, T>(val);
+	}
+#endif
+
 	template <typename F, typename T>
 	void checkUpperLimit(const F& from) const
 	{

Foundation/src/Var.cpp

@@ -47,7 +47,9 @@ Var::Var(const char* pVal)
 
 Var::Var(const Var& other)
 #ifdef POCO_NO_SOO
-	: _pHolder(other._pHolder ? other._pHolder->clone() : 0)
+	:_pHolder((this != &other) ?
+		(other._pHolder ? other._pHolder->clone() : nullptr) :
+		other._pHolder)
 {
 }
 #else
@@ -70,9 +72,10 @@ Var& Var::operator = (const Var& rhs)
 	Var tmp(rhs);
 	swap(tmp);
 #else
-	if ((this != &rhs) && !rhs.isEmpty())
+	if (this == &rhs) return *this;
+	if (!rhs.isEmpty())
 		construct(rhs);
-	else if ((this != &rhs) && rhs.isEmpty())
+	else
 		_placeholder.erase();
 #endif
 	return *this;
@@ -330,8 +333,6 @@ void Var::empty()
 	delete _pHolder;
 	_pHolder = 0;
 #else
-	if (_placeholder.isLocal()) this->~Var();
-	else delete content();
 	_placeholder.erase();
 #endif
 }
@@ -343,8 +344,6 @@ void Var::clear()
 	delete _pHolder;
 	_pHolder = 0;
 #else
-	if (_placeholder.isLocal()) this->~Var();
-	else delete content();
 	_placeholder.erase();
 #endif
 }

Foundation/testsuite/src/AnyTest.cpp

@@ -13,8 +13,10 @@
 #include "CppUnit/TestSuite.h"
 #include "Poco/Exception.h"
 #include "Poco/Any.h"
+#include "Poco/SharedPtr.h"
 #include "Poco/Bugcheck.h"
 #include <vector>
+#include <memory>
 
 
 #if defined(_MSC_VER) && _MSC_VER < 1400
@@ -272,6 +274,85 @@ void AnyTest::testVector()
 }
 
 
+void AnyTest::testPlaceholder()
+{
+#ifndef POCO_NO_SOO
+	Placeholder<char> ph;
+	assertTrue(ph.isEmpty());
+	assertFalse(ph.isLocal());
+
+	char c = *ph.assignStack<char, char>(1);
+	assertTrue(1 == c);
+	assertFalse(ph.isEmpty());
+	assertTrue(ph.isLocal());
+
+	c = *ph.assignHeap<char, char>(2);
+	assertTrue(2 == c);
+	assertFalse(ph.isEmpty());
+	assertFalse(ph.isLocal());
+
+	ph.erase();
+	assertTrue(ph.isEmpty());
+	assertFalse(ph.isLocal());
+
+	if (sizeof(std::shared_ptr<int>) <= POCO_SMALL_OBJECT_SIZE)
+	{
+		Placeholder<std::shared_ptr<int>> sph;
+		assertTrue(sph.isEmpty());
+		assertFalse(sph.isLocal());
+
+		int i = **sph.assignStack<std::shared_ptr<int>, int*>(new int(1));
+		assertTrue(1 == i);
+		assertFalse(sph.isEmpty());
+		assertTrue(sph.isLocal());
+
+		i = **sph.assignHeap<std::shared_ptr<int>, int*>(new int(1));
+		assertTrue(1 == i);
+		assertFalse(sph.isEmpty());
+		assertFalse(sph.isLocal());
+	}
+
+	if (sizeof(Poco::SharedPtr<int>) <= POCO_SMALL_OBJECT_SIZE)
+	{
+		Placeholder<Poco::SharedPtr<int>> psph;
+		assertTrue(psph.isEmpty());
+		assertFalse(psph.isLocal());
+
+		int i = **psph.assignStack<Poco::SharedPtr<int>, int*>(new int(1));
+		assertTrue(1 == i);
+		assertFalse(psph.isEmpty());
+		assertTrue(psph.isLocal());
+
+		i = **psph.assignHeap<Poco::SharedPtr<int>, int*>(new int(1));
+		assertTrue(1 == i);
+		assertFalse(psph.isEmpty());
+		assertFalse(psph.isLocal());
+	}
+
+	if (sizeof(std::vector<int>) <= POCO_SMALL_OBJECT_SIZE)
+	{
+		Placeholder<std::vector<int>> vph;
+		assertTrue(vph.isEmpty());
+		assertFalse(vph.isLocal());
+
+		std::vector<int> inv{1,2,3};
+		std::vector<int> outv = *vph.assignStack<std::vector<int>, std::vector<int>>(inv);
+		assertTrue(inv == outv);
+		assertFalse(vph.isEmpty());
+		assertTrue(vph.isLocal());
+
+		outv.clear();
+		outv = *vph.assignHeap<std::vector<int>, std::vector<int>>(inv);
+		assertTrue(inv == outv);
+		assertFalse(vph.isEmpty());
+		assertFalse(vph.isLocal());
+	}
+
+	// ...
+#endif
+}
+
+
 void AnyTest::setUp()
 {
 }
@@ -298,6 +379,7 @@ CppUnit::Test* AnyTest::suite()
 	CppUnit_addTest(pSuite, AnyTest, testInt);
 	CppUnit_addTest(pSuite, AnyTest, testComplexType);
 	CppUnit_addTest(pSuite, AnyTest, testVector);
+	CppUnit_addTest(pSuite, AnyTest, testPlaceholder);
 
 	return pSuite;
 }

Foundation/testsuite/src/AnyTest.h

@@ -37,6 +37,8 @@ public:
 	void testComplexType();
 	void testVector();
 
+	void testPlaceholder();
+
 	void setUp();
 	void tearDown();
 	static CppUnit::Test* suite();

Foundation/testsuite/src/VarTest.cpp

@@ -29,6 +29,9 @@ using namespace Poco;
 using namespace Poco::Dynamic;
 
 
+namespace {
+
+
 class Dummy
 {
 public:
@@ -54,6 +57,8 @@ private:
 	int _val;
 };
 
+}
+
 
 VarTest::VarTest(const std::string& rName): CppUnit::TestCase(rName)
 {
@@ -3068,6 +3073,20 @@ void VarTest::testIterator()
 }
 
 
+void VarTest::testSharedPtr()
+{
+	Poco::SharedPtr<int> p = new int(42);
+	{
+		Var v;
+		v = p;
+		Var v1;
+		v = v1;
+		v1 = v;
+	}
+	assertTrue(p.referenceCount() == 1);
+}
+
+
 void VarTest::setUp()
 {
 }
@@ -3116,6 +3135,7 @@ CppUnit::Test* VarTest::suite()
 	CppUnit_addTest(pSuite, VarTest, testOrderedDynamicStructString);
 	CppUnit_addTest(pSuite, VarTest, testDynamicStructInt);
 	CppUnit_addTest(pSuite, VarTest, testOrderedDynamicStructInt);
+	CppUnit_addTest(pSuite, VarTest, testSharedPtr);
 	CppUnit_addTest(pSuite, VarTest, testArrayToString);
 	CppUnit_addTest(pSuite, VarTest, testArrayToStringEscape);
 	CppUnit_addTest(pSuite, VarTest, testStructToString);

Foundation/testsuite/src/VarTest.h

@@ -77,6 +77,7 @@ public:
 	void testUUID();
 	void testEmpty();
 	void testIterator();
+	void testSharedPtr();
 
 	void setUp();
 	void tearDown();