(sorry for the late entrance, was OOO the last two weeks) My vote goes to (3). (2) is a good intermediate step (e.g. for Qt 6, with Qt 7 converting to static_assert()). Remember that ctors are implicitly noexcept in C++, so you need to do actual work to make a type that's not nothrow_destructible. In Qt, incl. 3rd-party monster chromium, we have _two_ such types, QChildProcess and QScopedPropertyUpdateGroup. QtC has another two, in sqlite: ~AbstractThrowingSessionTransaction and ~AbstractThrowingTransaction.
So, by an educated guesstimation, this won't be an issue, and possibly catch mistakes, like when using a 3rd-party library type in QVariant. The other option (4) is to make it supported: have a QVariant::destroy<T>() function that's conditionally noexcept, so users can destroy the payload in a controlled environment instead of running into std::terminate() in ~QVariant() (throwing from a noexcept function will _not_ unroll the stack, so you might not even get logging that you expected). IMHO, (1) is not an acceptable option. Us C++ professionals having identified this problem after years of it lying dormant, it behooves us, at the very least, to educate our users about this, e.g. by adding docs, and maybe a qWarning() in ~QVariant(), if we don't do (2). Thanks, Marc On 09.08.24 10:46, Fabian Kosmale via Development wrote: > Hi, > > Qt's container classes (at least those listed in > https://doc.qt.io/qt-6/containers.html + QCache) reject types with > throwing destructorss via static_assert[1]. > > There is however one "container" which doesn't reject such types: > QVariant. Even though it's own destructor is noexcept, it doesn't reject > types whose destructor is potentially throwing. My question is: Do we > want to change this, and if so, how? There are at least three alternatives: > > 1. Don't do anything; that's the behavior we have since at least Qt 5. > If the dtor doesn't actually throw, everything is fine; if it does > throw, we're calling std::terminate. Might lead to unexpected results, > but doing nothing obviously doesn't break any existing code. It is > however inconsistent with other types. > 2. Warn, but still accept such types [2]. I've implemented that in > https://codereview.qt-project.org/c/qt/qtbase/+/580560. Anyone one > stores a type with a throwing dtor would get a warning, but everything > still compiles. The warning can however be considered spurious if the > author knows that given their usage, the type would never throw. > 3. Reject such types at compile time. That would be consistent with > other types, but might break existing code, even code that works > perfectly fine because it can never actually throw. > > I'm leaning towards either 1 (nobody ever reported a bug about QVariant > calling std::terminate so far), or 2 ( more consistent with our policy > for containers, highlights that such usage is potentially dangerous). > > I'd welcome any comments on concerns. > > Kind regards, > Fabian > > [1] There are however still some methods which are marked as > conditionally noexept, like QCache::take, even though that can never be > the case given the static_assert. > [2] At least in cases where we can detect it at compile time; the API > using QMetaType would require storing the necessary information in > QMetaType, and a runtime check. > -- Marc Mutz <marc.m...@qt.io> (he/his) Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B -- Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
