Don't allow storing types that throw in the destructor in our containers

Types that throw in their destructors are strongly discouraged in C++,
and even the STL doesn't define what happens if such types are stored
in their containers.

Make this more explicit for Qt and disallow storing those types in our
containers. This will hopefully preempty any potential future bug
reports about us not handling such a case. It also helps simplify
some code in QList and other cases and makes it possible to explicitly
mark more methods as noexcept.

Some care needs to be taken where to add the static asserts, so that
we don't disallow forward declarations of types stored in containers.
Place the static assert into the destructor of the container where
possible or otherwise into the templated d-pointer.

Change-Id: If3aa40888f668d0f1b6c6b3ad4862b169d31280e
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

src/corelib/tools/qarraydataops.h

@@ -223,6 +223,8 @@ template <class T>
 struct QPodArrayOps
         : public QArrayDataPointer<T>
 {
+    static_assert (std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
 protected:
     typedef QTypedArrayData<T> Data;
 
@@ -462,6 +464,8 @@ template <class T>
 struct QGenericArrayOps
         : public QArrayDataPointer<T>
 {
+    static_assert (std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
 protected:
     typedef QTypedArrayData<T> Data;
 
@@ -892,6 +896,8 @@ template <class T>
 struct QMovableArrayOps
     : QGenericArrayOps<T>
 {
+    static_assert (std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
 protected:
     typedef QTypedArrayData<T> Data;
 

src/corelib/tools/qcache.h

@@ -193,8 +193,15 @@ class QCache
 public:
     inline explicit QCache(qsizetype maxCost = 100) noexcept
         : mx(maxCost)
-    {}
+    {
-    inline ~QCache() { clear(); }
+    }
+    inline ~QCache()
+    {
+        static_assert(std::is_nothrow_destructible_v<Key>, "Types with throwing destructors are not supported in Qt containers.");
+        static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
+        clear();
+    }
 
     inline qsizetype maxCost() const noexcept { return mx; }
     void setMaxCost(qsizetype m) noexcept(std::is_nothrow_destructible_v<Node>)

src/corelib/tools/qcontiguouscache.h

@@ -73,6 +73,8 @@ struct QContiguousCacheTypedData : public QContiguousCacheData
 
 template<typename T>
 class QContiguousCache {
+    static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
     typedef QContiguousCacheTypedData<T> Data;
     Data *d;
 public:

src/corelib/tools/qflatmap_p.h

@@ -109,6 +109,8 @@ template<class Key, class T, class Compare = std::less<Key>, class KeyContainer
          class MappedContainer = QList<T>>
 class QFlatMap : private QFlatMapValueCompare<Key, T, Compare>
 {
+    static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
     using full_map_t = QFlatMap<Key, T, Compare, KeyContainer, MappedContainer>;
 
     template <class U>

src/corelib/tools/qhash.h

@@ -725,6 +725,9 @@ public:
     }
     ~QHash()
     {
+        static_assert(std::is_nothrow_destructible_v<Key>, "Types with throwing destructors are not supported in Qt containers.");
+        static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
         if (d && !d->ref.deref())
             delete d;
     }
@@ -1191,6 +1194,9 @@ public:
     }
     ~QMultiHash()
     {
+        static_assert(std::is_nothrow_destructible_v<Key>, "Types with throwing destructors are not supported in Qt containers.");
+        static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
         if (d && !d->ref.deref())
             delete d;
     }

src/corelib/tools/qmap.h

@@ -68,6 +68,9 @@ public:
     using iterator = typename Map::iterator;
     using const_iterator = typename Map::const_iterator;
 
+    static_assert(std::is_nothrow_destructible_v<Key>, "Types with throwing destructors are not supported in Qt containers.");
+    static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
     Map m;
 
     QMapData() = default;
@@ -217,7 +220,8 @@ public:
 template <class Key, class T>
 class QMap
 {
-    using MapData = QMapData<std::map<Key, T>>;
+    using Map = std::map<Key, T>;
+    using MapData = QMapData<Map>;
     QtPrivate::QExplicitlySharedDataPointerV2<MapData> d;
 
     friend class QMultiMap<Key, T>;
@@ -446,8 +450,8 @@ public:
         friend class QMap<Key, T>;
         friend class const_iterator;
 
-        typename MapData::Map::iterator i;
+        typename Map::iterator i;
-        explicit iterator(typename MapData::Map::iterator it) : i(it) {}
+        explicit iterator(typename Map::iterator it) : i(it) {}
     public:
         typedef std::bidirectional_iterator_tag iterator_category;
         typedef qptrdiff difference_type;
@@ -491,8 +495,8 @@ public:
     class const_iterator
     {
         friend class QMap<Key, T>;
-        typename MapData::Map::const_iterator i;
+        typename Map::const_iterator i;
-        explicit const_iterator(typename MapData::Map::const_iterator it) : i(it) {}
+        explicit const_iterator(typename Map::const_iterator it) : i(it) {}
 
     public:
         typedef std::bidirectional_iterator_tag iterator_category;
@@ -745,7 +749,8 @@ Q_DECLARE_MUTABLE_ASSOCIATIVE_ITERATOR(Map)
 template <class Key, class T>
 class QMultiMap
 {
-    using MapData = QMapData<std::multimap<Key, T>>;
+    using Map = std::multimap<Key, T>;
+    using MapData = QMapData<Map>;
     QtPrivate::QExplicitlySharedDataPointerV2<MapData> d;
 
 public:
@@ -1062,8 +1067,8 @@ public:
         friend class QMultiMap<Key, T>;
         friend class const_iterator;
 
-        typename MapData::Map::iterator i;
+        typename Map::iterator i;
-        explicit iterator(typename MapData::Map::iterator it) : i(it) {}
+        explicit iterator(typename Map::iterator it) : i(it) {}
     public:
         typedef std::bidirectional_iterator_tag iterator_category;
         typedef qptrdiff difference_type;
@@ -1107,8 +1112,8 @@ public:
     class const_iterator
     {
         friend class QMultiMap<Key, T>;
-        typename MapData::Map::const_iterator i;
+        typename Map::const_iterator i;
-        explicit const_iterator(typename MapData::Map::const_iterator it) : i(it) {}
+        explicit const_iterator(typename Map::const_iterator it) : i(it) {}
 
     public:
         typedef std::bidirectional_iterator_tag iterator_category;

src/corelib/tools/qvarlengtharray.h

@@ -60,6 +60,8 @@ QT_BEGIN_NAMESPACE
 template<class T, qsizetype Prealloc>
 class QVarLengthArray
 {
+    static_assert(std::is_nothrow_destructible_v<T>, "Types with throwing destructors are not supported in Qt containers.");
+
 public:
     QVarLengthArray() : QVarLengthArray(0) {}
 

tests/auto/corelib/tools/collections/tst_collections.cpp

@@ -2920,6 +2920,7 @@ void tst_Collections::forwardDeclared()
     TEST(QVector<T1>);
     TEST(QStack<T1>);
     TEST(QQueue<T1>);
+    TEST(QSet<T1>);
 #undef TEST
 #undef COMMA
 

tests/auto/corelib/tools/qarraydata/tst_qarraydata.cpp

@@ -2297,7 +2297,6 @@ ThrowingTypeWatcher& throwingTypeWatcher() { static ThrowingTypeWatcher global;
 struct ThrowingType
 {
     static unsigned int throwOnce;
-    static unsigned int throwOnceInDtor;
     static constexpr char throwString[] = "Requested to throw";
     static constexpr char throwStringDtor[] = "Requested to throw in dtor";
     void checkThrow()  {
@@ -2326,22 +2325,14 @@ struct ThrowingType
         checkThrow();
         return *this;
     }
-    ~ThrowingType() noexcept(false)
+    ~ThrowingType()
     {
         throwingTypeWatcher().destroyed(id);  // notify global watcher
         id = -1;
 
-        // deferred throw
-        if (throwOnceInDtor > 0) {
-            --throwOnceInDtor;
-            if (throwOnceInDtor == 0) {
-                throw std::runtime_error(throwStringDtor);
-            }
-        }
     }
 };
 unsigned int ThrowingType::throwOnce = 0;
-unsigned int ThrowingType::throwOnceInDtor = 0;
 bool operator==(const ThrowingType &a, const ThrowingType &b) {
     return a.id == b.id;
 }