[Development] RFC: QVariant changes in Qt6

[Eike Ziller]

> On 24. Nov 2019, at 18:57, Olivier Goffart <olivier at woboq.com> wrote:
> 
> On 24.11.19 12:36, Lars Knoll wrote:
>> Hi Olivier,
>> Thanks for looking through this and coming up with a proposal. I like the direction.
>>> On 22 Nov 2019, at 14:32, Olivier Goffart <olivier at woboq.com> wrote:
>>> 
>>> Hi,
>>> 
>>> This is a follow-up on what was discussed in the (second part of the) QtCore session in the QtCS.
>>> Lars and others have been mentioning that they dislike implicit conversions within QVariant. Creating a new class (QAny) has been suggested, that would be like QVariant but without the conversions.
>>> I am personally not in favor of this change because we are using QVariant all over the place in our API and so we cannot really deprecate QVariant. It will cause much confusion to user to have two similar classes. And the difference is not big enough to force a new class.
>>> 
>>> So here is what I suggest we do in Qt6. None of this is implemented yet, it is only proposed on this list for feedback.
>>> 
>>> 
>>> 1. operator==
>>> 
>>> In Qt6, QVariant::operator==() will no longer do any conversions.
>>> If two QVariant does not have exactly the same, they will no longer be considered equal.
>>> Also, in Qt6, QMetatype will gain ability to register the operator==, and therefore it will work for any type (and not only for builtin type as currently).
>>> 
>>> So right now,
>>>   QVariant(QByteArray("Hello")) == QVariant(QString("Hello"))
>>> is true, but in Qt6 it will be false.
>>> 
>>> This is a behavior change, but I believe this is something we can afford to do.
>>> I do not have data on how much code will break with this change, but i feel most use of operator== are there for optimisations: i.e:   setFoo(const QVariant &foo) { if (m_foo == foo) return; ... }
>>> Maybe we'll have more data once we actually implement the change and see if too many things breaks.
>> This should be relatively uncontroversial. The current behaviour is at the very least unexpected by users.
>>> 
>>> 
>>> 2. operator< and other comparison operator
>>> 
>>> Deprecate in Qt 5.15, remove in Qt 6
>>> 
>>> It is not possible to implement it correctly with a total order.
>>> 
>>> I could not find direct use of the operator in the code indexed on https://code.woboq.org/qt5 (only in QmlDesigner::operator< which is itself not used)
>>> Sorting on variant does not really make sense. Even code that does, like QAbstractItemModelPrivate::isVariantLessThan does not use operator<.
>>> 
>>> Where this is used may be the use of QVariant as a key in a QMap. This is problematic because the operator< does not implement a total order, so you can have funny results.
>>> I could not find instances of that in Qt or QtCreator, but Github search for "QMap<QVariant," shows many result :-(
>>> I'd still want to deprecate it. User could wrap QVariant in their own MySortedVariant with their own operators that does what they need for their use case.
>> +1. Total ordering will only work in special cases, as we do not control the types of data stored in a QVariant. Using a QMap<QVariant, …> sounds like a design mistake in any case. Let’s deprecate and remove in Qt 6.
> 
> Deprecation MR: https://codereview.qt-project.org/282432
> 
> Other things came up, QMetaType::registerComparators also register the operator<, but i suggest leaving it as it in 5.15 (as it is usefull to register operator==) and make it a deprecated no-op in Qt6
> 
> There is also QMetaType::compare which has then to be removed in Qt6. I couldn't find any use of this function on github that was not some automaticaly generated bindings, or the tests within Qt itself. I guess it's Ok to deprecate it in Qt 5.15 as well even if there is no replacement. Or would it be ok to just remove it.
> 
>>> 3. conversions in QVariant::value
>>> 
>>> We would like to avoid having automatic conversion in QVariant::value.
>>> So Qt6 would be
>>>   std::optional<T> QVariant::value() const;
>>> And we could deprecate the current one in Qt5.15 in favor of qvariant_cast which is explicit.
>>> 
>>> This one is a bit more controversial maybe. Because there are thousands of call to QVariant::value all over the place. But "value()" is the ideal name for the non-converting variant.
>>> A clazy script to replace QVariant::value with qvariant_cast will be in order.
>> I like this idea. value() is the perfect API to return the value of the variant without implicit conversions. The advantage of the above approach is that it offers a way to already migrate in Qt 5 and will give compile errors in Qt6 for code that hasn’t been migrated to use qvariant_cast.
>> One more idea here: qvariant_cast was done as a free standing function because of limitations in VC++ 6. We could also add a template<typename T> QVariant::cast<T>() (or maybe to() or convertedTo()). IMO that would make the code look a bit nicer than using the freestanding qvariant_cast method.
> 
> The VC++6 workaround was called qVariantValue (and qVariantFromValue, qVariantSetValue). I think qvariant_cast was done on purpose independently.
> Adding a cast<T> function does not sound like a bad idea? But should it be done in Qt5.15 or Qt6. And should it return T or optional<T> (in case the conversion did not work)  or have a bool*ok=nullptr  parameter?
> 
>>> 4. All the implicit constructors for builtin types.
>>> 
>>> QVariant has many implicit constructors for all the builtin types.
>>> I suggest to replace them all with a template<typename T> QVariant(T&&) constructor. (same as std::any.) So builtin types are no longer special.
>> +1. This should be source compatible.
>>> 
>>> 
>>> 5. All the method toXxx (where Xxx is a builtin type)
>>> 
>>> Leave them as-is?
>>> However some of them are for types that may go outside of QtCore, these should be deprecated in Qt 5.15 and removed in Qt6
>> They do basically the same as qvariant_cast, so we could simply deprecate them all and replace by the cast() method mentioned above.
> 
> The issue is that there is lots of use of these (esp. the most common ones like toString and toInt.) Removing all uses be a huge work for no obvious reasons.
> (And i was told they are now recommended by clazy)

_If_ you “deprecate all toXxx”, please keep qvariant_cast (without deprecation). If you also replace qvariant_cast with QVariant::cast, the proposed replacement for the deprecated methods is introduced late in the process, which makes porting harder, and as Olivier says, for no obvious reason.
qvariant_cast also is consistent with the other “casts” in Qt and C++.

> 
>>> 6. QVariant::Type and QMetaType::Type enums
>>> 
>>> QVariant::Type is already marked as obsolete in the documentation, but not yet marked as deprecated.
>>> So we can remove it in Qt6, and we should try to mark it as deprecated in Qt 5.15. But that's hard because it is used all over the place.
>>> 
>>> QMetaType::Type will be marked as deprecated in Qt6, but i'm afraid we cannot simply remove it.
>>> In  general, all the integer id API for QMetaType will be deprecated in Qt6, one should use QMetaType by value. The integer id will stay in Qt6. This means that there will still be a central registry of types but it would only be there for the types for which we ask the id (and for the builtin types)
>> Sounds ok to me.
>> Cheers,
>> Lars
> 
> _______________________________________________
> Development mailing list
> Development at qt-project.org
> https://lists.qt-project.org/listinfo/development

-- 
Eike Ziller
Principal Software Engineer

The Qt Company GmbH
Erich-Thilo-Straße 10
D-12489 Berlin
eike.ziller at qt.io
http://qt.io
Geschäftsführer: Mika Pälsi,
Juha Varelius, Mika Harjuaho
Sitz der Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB 144331 B