Hi, (back from vacation)
On 08.04.26 10:27, Volker Hilsheimer via Development wrote: Having followed the discussion on gerrit for a while, I agree with the following points made there: - QMetaObject::Connection is a handle- or pointer-like type There is no reason for it to become move-only (it’s not an owning handle); and as with all handles, the handled object might change even if the handle itself is const. This might result in a change of an observable state. Connection is a shared_ptr when it could be a unique_ptr, that's all. There are very few existing users that actually _require_ copying. Mostly, it's missing std::move()s and use of QList and QHash instead of QVLA, std::vector, or std::unordered_map, all of which are fixable, and tend to improve the codegen. There's overhead in shared_ptr vs. the unique_ptr, and if we were to implement this API anew now, we would certainly make it move-only to begin with. Copyability is easily added on a per-use case with a shared_ptr. Don't pay for what you don't use. There's also the question of the thread-safety of disconnect(Connection), which would also be trivial for a move-only type, but require atomic operations for a copyable one. So no, we don't _need_ to make Connection move-only, but it would make the type easier to grok for new users and make code easier to follow, because there's only ever one owner. Taking care not to confuse familiarity with simplicity, a move-only Connection is therefore just better API. Given we strive for good API, the question isn't whether Connection should be copyable (it shouldn't be), but whether there's a realistic path to making it move-only. Thus my suggestion to add the opt-in macro to disable copyability, add it to STRICT_MODE, and then, in Qt 7 and Qt 8, see whether we can make it permanent. - making the modified member mutable strikes me as the best tradeoff: it solves the UB, while not forcing people to modify working and compiling code That is essentially https://codereview.qt-project.org/c/qt/qtbase/+/721918, once the comments to the commit message are taken into account. I don't disagree. I'm just saying this bug is special in that it can be fixed on the user side. Assuming the project is in a regulated industry, changing user code ought to be easier than changing the Qt version. Also, Qt 5 users won't get a new Qt version, but they can still fix their code. Not many bugs are like that. The absence of a need for a patched Qt makes this special, thus I raised it on the MLs. We have zero data points suggesting that any current compiler would optimize the clearing of the member away, or even optimize out the actual disconnect. Nobody has so far produced any (however contrived, such as above) scenario in which this happens. IOW, the risk of this issue causing any real problems is practically zero. This is not how UB works, unfortunately. Ask the Linux kernel guys about GCC silently removing their buffer overflow guards some years back :( We would also not necessarily see bug reports, because the likely effect is that a disconnect() doesn't happen at the manually-specified time, but only later, at source or receiver object destruction, and the likely problem there is further UB such as invalid downcasts, of which we had many in Qt and probably continue to have many, without bug reports about them, either. The main argument repeated in the discussion is that this fix requires a recompile as well as Qt upgrade. But given that the issue evidently only materializes with a hypothetical future compiler, a recompile would be needed for that to happen anyway. So that argument seems to bite its own tail. The question is what to do though. I'm saying that we should do everything we can, but reviewers stop any one of the patches because it's not their favourite solution. IMNSHO, we should: - document that disconnect() modifies the argument - publicize that just not passing const objects makes you immune (started here) - add non-const overloads - add an opt-out that disables the const ones - make the member mutable, add a change-log that the alternative to recompilation is to not pass const objects - add an opt-out that disables copying (all the above can presumably be done in all active branches) - deprecate the const overloads - deprecate copying - remove the const overloads - remove copying - un-mutable the member - make/mark disconnect(Connection) thread-safe This gives users the most flexibility, and lets both the Qt project and its users fall into the pit of success. Thanks, Marc -- Marc Mutz <[email protected]><mailto:[email protected]> (he/his) Principal Software Engineer The Qt Company Erich-Thilo-Str. 10 12489 Berlin, Germany www.qt.io<http://www.qt.io> Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B Confidential
-- Development mailing list [email protected] https://lists.qt-project.org/listinfo/development
