Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Monday 8. June 2015 17:57:23 Thiago Macieira wrote:
On Monday 08 June 2015 15:58:23 Olivier Goffart wrote:
There is no reason to stop improving qmetatype.
The qFatal was there for a good reason.
It was there for a good reason for the existing flags.
But for new flags of course it does not make sens.
It did make sense: the idea was that registering new flags would cause the
very incompatibility we're seeing here. I'm not entirely convinced that we
discussed all scenarios at QtCS, so I'm still skeptical about allowing the
IsGadget flag. I insist that we \omitvalue for now, until we understand the
consequences better.
No, the qFatal is there because a change in the other flags like the
MovableType is binary incompatible.
A change in the IsGadget flag is fine.
Old code did not need this flag. And code that relies on it requires anyway
that the code that register the types register the isGadget flag.
Even if one could build a complicated use case that breaks, it would be very
unlikely to happen.
And any change is dangerous. When we fix any small bug, there can always be
applications that breaks because it was relying on the bug for something.
Anyway, let us summarize the problem in that case.
In Qt = 5.4, QtLocation was using QML Private API to basically creates
QObject wrapper around QGeoCoordinate so it can be exposed in QML. But in Qt
5.5, one does not need private API, everybody can expose value types to QML
just by adding Q_GADGET to it and registering Q_PROPERTY or Q_INVOKABLE.
So QGeoCoordinate became a Q_GADGET. But for QML to be able to take advantage
of it, the type needs to be registered with the QMetaType::IsGadget flag. This
is automatic. But if the metatype is registered by code compiled with Qt 5.4
or before, the IsGadget flag is not present.
This is what is happening in a application that was compiled against Qt 5.4,
QGeoCoordinate was used in signal and slot and registered as a metatype by the
application. QMetaType will detect the difference in the flags on the second
registration and do a qFatal.
The solution is obviously not to do a qFatal, but take the new flags in
addition. And then everything works as expected.
Now the problem is: could there be code that relies in IsGadget that is run
and that the type is only registered by old code but not by new code. i.e:
could loading an old plugin compiled with an old code suddenly break a new
application using these new feature.
In theory this is possible, but in practice I don't think this will ever
happen.
Let's suppose an application called App 1.0 which has this code:
mystruct.h:
struct MyStruct {
int myValue;
};
Q_DECLARE_METATYPE(MyStruct);
myitem_p.h
class MyItem : public QObject {
Q_OBJECT
Q_INVOKABLE extractValue(const QVariant v) {
return qvariant_castMyStruct(v).myValue;
}
};
foo.qml:
MyItem {
function foobar(someObject) {
return extractValue(someObject.myStructProperty);
}
}
someObject is an object coming from a plugin which has a property
myStructProperty of type MyStruct.
Now, App 1.1 gets released and they simplify the code:
mystruct.h:
struct MyStruct {
Q_GADGET;
Q_PROPERTY(int myValue MEMBER myValue)
public:
int myValue;
};
foo.qml:
MyItem {
function foobar(someObject) {
// now that myStruct is a Q_GADGET i don't need the helper
return someObject.myStructProperty.myValue;
}
}
Then yes, if the MyStruct was only registered by the plugin we get a binary
compatibility problem in the application. But this is a problem for the
application, not Qt. And the solution is to make sure that MyStruct is
registered by manually registering it using qRegisterMetaType
And it's not just the flag. I'm not convinced about the template detection
either. You had to apply two late fixes to the detection so that we wouldn't
break source compatibility or create unnecessary warnings.
Yes, I had to apply fixes after the beta was released and it got tested in the
wild. But is that not what beta releases are for?
The freeze stays: no new flags in QMetaType until Qt 6, no more messing
with the template black magic.
You can't mandate that.
Yes, I can. As the maintainer, I have the authority and mandate to oversee
the changes to the module I maintain and that includes blocking changes I
am unsatisfied with.
A mailing list consensus can overrule me, as can the Chief Maintainer.
We stay frozen until further notice. If you have new flags you want to
propose, you can do it, but we'll need a mailing list discussion before the
change is allowed.
Well, then it will be reviewed as usual.
There are few improvements that can be done to QMetaType that we were
discussing at the summit, like the ability to modify list types (append and
such) or including some of the C++11 features. I bet one can simplify
qmetatype.h when we require C++11's decltype and proper SFINAE rules
Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Tuesday 09 June 2015 12:10:02 Olivier Goffart wrote: A change in the IsGadget flag is fine. Old code did not need this flag. And code that relies on it requires anyway that the code that register the types register the isGadget flag. Even if one could build a complicated use case that breaks, it would be very unlikely to happen. And yet it did happen, or we wouldn't be having this conversation. If Alex could do it in qtconnectivity, why can't others do it in other circumstances? And any change is dangerous. When we fix any small bug, there can always be applications that breaks because it was relying on the bug for something. The problem is the associated risk because this code is complex and varies depending on factors we usually don't think about. This isn't about your standard class that operates on data. Anyway, let us summarize the problem in that case. In Qt = 5.4, QtLocation was using QML Private API to basically creates QObject wrapper around QGeoCoordinate so it can be exposed in QML. But in Qt 5.5, one does not need private API, everybody can expose value types to QML just by adding Q_GADGET to it and registering Q_PROPERTY or Q_INVOKABLE. So QGeoCoordinate became a Q_GADGET. But for QML to be able to take advantage of it, the type needs to be registered with the QMetaType::IsGadget flag. This is automatic. But if the metatype is registered by code compiled with Qt 5.4 or before, the IsGadget flag is not present. This is what is happening in a application that was compiled against Qt 5.4, QGeoCoordinate was used in signal and slot and registered as a metatype by the application. QMetaType will detect the difference in the flags on the second registration and do a qFatal. The solution is obviously not to do a qFatal, but take the new flags in addition. And then everything works as expected. And register the metatype for the Q_GADGET, which it hadn't done before. My point is that you had not thought of this when you wrote the feature. Therefore, there shall be no more such features without major discussion because we are failing to see all the possible consequences. And I am not convinced yet we've found them all. What are the consequences of overriding the flag and the metatype? Could it be that the user did intentionally break compatibility, yet failed to recompile one module? That's what the qFatal was there for: a reminder. We're removing it. And it's not just the flag. I'm not convinced about the template detection either. You had to apply two late fixes to the detection so that we wouldn't break source compatibility or create unnecessary warnings. Yes, I had to apply fixes after the beta was released and it got tested in the wild. But is that not what beta releases are for? The problem is the precedent. We are not finding everything and this code is extremely complex. The more we add to it, the more complex it becomes (exponentially so). Well, then it will be reviewed as usual. There are few improvements that can be done to QMetaType that we were discussing at the summit, like the ability to modify list types (append and such) or including some of the C++11 features. I bet one can simplify qmetatype.h when we require C++11's decltype and proper SFINAE rules We will have to discuss each modification in the mailing list. And note that replacing the rules with C++11 simpler rules may have side- effects too, so you should not simplify for the sake of simplification. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Monday 08 June 2015 15:58:23 Olivier Goffart wrote: There is no reason to stop improving qmetatype. The qFatal was there for a good reason. It was there for a good reason for the existing flags. But for new flags of course it does not make sens. It did make sense: the idea was that registering new flags would cause the very incompatibility we're seeing here. I'm not entirely convinced that we discussed all scenarios at QtCS, so I'm still skeptical about allowing the IsGadget flag. I insist that we \omitvalue for now, until we understand the consequences better. And it's not just the flag. I'm not convinced about the template detection either. You had to apply two late fixes to the detection so that we wouldn't break source compatibility or create unnecessary warnings. The freeze stays: no new flags in QMetaType until Qt 6, no more messing with the template black magic. You can't mandate that. Yes, I can. As the maintainer, I have the authority and mandate to oversee the changes to the module I maintain and that includes blocking changes I am unsatisfied with. A mailing list consensus can overrule me, as can the Chief Maintainer. We stay frozen until further notice. If you have new flags you want to propose, you can do it, but we'll need a mailing list discussion before the change is allowed. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Saturday 6. June 2015 11:17:30 Thiago Macieira wrote: On Friday 05 June 2015 10:11:28 Frederik Gladhorn wrote: I can't tell if the qmetatype.h template magic is binary compatible or not. As evidenced by a recent push [1] about the IsGadget flag, it isn't. I'm going to put a stop to this. qmetatype.h template magic is now frozen as of Qt 5.5 and until Qt 6.0, aside from bugfixes or pure additions that do not modify existing template classes. [1] https://codereview.qt-project.org/113652 The problem here is the qFail. We can just remove the qFail in this case (what the patch does) and we will be fine. There is no reason to stop improving qmetatype. Q_DECLARE_FLAGS(LoadHints, LoadHint) +Q_FLAG(LoadHint) +Q_FLAG(LoadHints) Do we need both Q_FLAG? That looks like a mistake. The code was just moved from Q_FLAGS to Q_FLAG. Both were there before and I kept it like that. It cannot really be changed otherwise code like staticQtMetaObject.indexOfEnumerator(LoadHint) or staticQtMetaObject.indexOfEnumerator(LoadHints) will break. The use of Q_ENUMS and Q_FLAGS with QFlags types is not really consistent within Qt. -- Olivier Woboq - Qt services and support - http://woboq.com - http://code.woboq.org ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Monday 8. June 2015 09:34:30 Thiago Macieira wrote: On Monday 08 June 2015 11:28:48 Olivier Goffart wrote: On Saturday 6. June 2015 11:17:30 Thiago Macieira wrote: On Friday 05 June 2015 10:11:28 Frederik Gladhorn wrote: I can't tell if the qmetatype.h template magic is binary compatible or not. As evidenced by a recent push [1] about the IsGadget flag, it isn't. I'm going to put a stop to this. qmetatype.h template magic is now frozen as of Qt 5.5 and until Qt 6.0, aside from bugfixes or pure additions that do not modify existing template classes. [1] https://codereview.qt-project.org/113652 The problem here is the qFail. We can just remove the qFail in this case (what the patch does) and we will be fine. There is no reason to stop improving qmetatype. The qFatal was there for a good reason. It was there for a good reason for the existing flags. But for new flags of course it does not make sens. The freeze stays: no new flags in QMetaType until Qt 6, no more messing with the template black magic. You can't mandate that. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Monday 08 June 2015 11:28:48 Olivier Goffart wrote: On Saturday 6. June 2015 11:17:30 Thiago Macieira wrote: On Friday 05 June 2015 10:11:28 Frederik Gladhorn wrote: I can't tell if the qmetatype.h template magic is binary compatible or not. As evidenced by a recent push [1] about the IsGadget flag, it isn't. I'm going to put a stop to this. qmetatype.h template magic is now frozen as of Qt 5.5 and until Qt 6.0, aside from bugfixes or pure additions that do not modify existing template classes. [1] https://codereview.qt-project.org/113652 The problem here is the qFail. We can just remove the qFail in this case (what the patch does) and we will be fine. There is no reason to stop improving qmetatype. The qFatal was there for a good reason. The freeze stays: no new flags in QMetaType until Qt 6, no more messing with the template black magic. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Qt 5.5.0 header diff: QtCore.diff
On Friday 05 June 2015 10:11:28 Frederik Gladhorn wrote: I can't tell if the qmetatype.h template magic is binary compatible or not. As evidenced by a recent push [1] about the IsGadget flag, it isn't. I'm going to put a stop to this. qmetatype.h template magic is now frozen as of Qt 5.5 and until Qt 6.0, aside from bugfixes or pure additions that do not modify existing template classes. [1] https://codereview.qt-project.org/113652 +++ b/src/corelib/plugin/qfactoryinterface.h Q_DECLARE_FLAGS(LoadHints, LoadHint) +Q_FLAG(LoadHint) +Q_FLAG(LoadHints) Do we need both Q_FLAG? That looks like a mistake. +// ### Qt6: check if there's a better way +class QStringList : public QListQString [...] -inline void sort(Qt::CaseSensitivity cs = Qt::CaseSensitive); -inline int removeDuplicates(); The above works only because: 1) QStringList is not exported 2) the functions in question are inlines However, if someone derives from QStringList and exports a class, using MSVC, then this may be binary incompatible. See qvector_msvc.h. I have not and will not investigate this further, so I don't know if this is real or not. If there's a bug reported on this, I will probably close it with you're committing too many mistakes (using MSVC and deriving from QStirngList). The rest looks good, but I hope I didn't miss anything in the Q_DECL_NOTHROW, Q_DECL_OVERRIDE and Q_DECL_CONSTEXPR noise. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
