[Development] QVariant and types with throwing dtors

[Marc Mutz]

(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.mutz at 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