Re: [Development] Source break policy for function overloads
On 2016-07-13 23:55, Lars Knoll wrote: On 13 Jul 2016, at 20:10, Marc Mutz wrote: [...] It should also be noted that there are two categories of SiCs: a. those that can be fixed client-side without breaking compat with older Qt versions, and b. those which cannot IMO, SiCs of type (a) are acceptable, those of type (b) are obviously no-no's. Adding an overload (both cases: 1 + 2 above) is type (a), so is acceptable. Adding explicit to a ctor that should have been but wasn't is also SiC Type (a). In these cases, code that breaks is code that deserves to be broken, because it was brittle. Renaming a public inline function of a non-exported class is BC, but SiC Type (b), so not acceptable. I think Marc has summed it up very nicely. This is IMO exactly the policy we should apply. QUIP 6 has been created from this: https://codereview.qt-project.org/182311 ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
> On Jul 13, 2016, at 9:47 AM, Thiago Macieira > wrote: > > Em quarta-feira, 13 de julho de 2016, às 10:44:13 PDT, Alexander Blasche > escreveu: >> Hi, >> >> Yesterday, I stumbled over a break in QtSerialBus due to the addition of new >> QTimer::setInterval(..) overloads in qtbase/dev. This was introduced by >> >> https://codereview.qt-project.org/#/c/160889/ >> >> Unfortunately this has the undesirable side effect that lines like: >> >> q->connect(q, &QModbusClient::timeoutChanged, element.timer.data(), >> &QTimer::setInterval); >> >> do not compile anymore. The function pointer to setInterval() becomes >> ambiguous. While the fix (https://codereview.qt-project.org/#/c/165034/) >> was not difficult once you know what is happening this can hit every >> customer and basically reduces the source compatibility promise into >> absurdity. >> >> While talking to several devs in the office I could not find a congruent >> opinion on this issue. Do we or even can we care? Do we have a policy? > > We do have this policy since 5.0 that we have to be careful about overloads > due to the connect syntax. The breakage was accidental and not intended. > > I did intend to add overloads, as they're nice to use, I just hadn't > considered the consequences. > While I agree that we must be careful when adding new overloads, I think we can do better with QObject::connect as well. I played around with this patch today, it fixes the breakage that sparked this thread without any other modifications: https://codereview.qt-project.org/165505 There are obviously things to improve in this WIP patch, but I do believe we can make improvements by hinting to the compiler which member function it should choose. I’ll explore this some more unless someone has a clear reason not to go down this road. Cheers, Louai smime.p7s Description: S/MIME cryptographic signature ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
Em quarta-feira, 13 de julho de 2016, às 23:29:42 PDT, Richard Moore escreveu: > On 13 July 2016 at 19:10, Marc Mutz wrote: > > Renaming a public inline function of a non-exported class is BC, but SiC > > Type > > (b), so not acceptable. > > Good analysis, however isn't the compiler allowed to not inline stuff too > which would mean this example is not b/c either. Indeed, but compilers need to emit the out-of-line copy if they decided not to inline. That means the previous copy exists for code that wasn't recompiled, regardless of whether the class has changed since. That is why it's BC but not SC. This applies even for MSVC, for non-exported classes (Marc's case). For exported classes, MSVC in debug mode (unlike the IA-64 C++ ABI) calls the out- of-line copy without emitting it, which the DLL that exported the class must have emitted. That is why removing inline functions in exported classes is not BC. It's also why MSVC exports too much from template classes, if you derive from it and export (QPolygon and QVector case). I believe it has a nasty Level 4 warning, about dllexporting a class that isn't dllexported, but we have turned it off for at least 10 years. -- 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] Source break policy for function overloads
On 13 July 2016 at 19:10, Marc Mutz wrote: > Renaming a public inline function of a non-exported class is BC, but SiC > Type > (b), so not acceptable. > Good analysis, however isn't the compiler allowed to not inline stuff too which would mean this example is not b/c either. Cheers Rich. ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
On Wednesday 13 July 2016 18:47:28 Thiago Macieira wrote: > We do have this policy since 5.0 that we have to be careful about > overloads due to the connect syntax. The breakage was accidental and not > intended. > > I did intend to add overloads, as they're nice to use, I just hadn't > considered the consequences. When I reported an SC breakage from an added overload in Qt 4, I got back a WONTFIX. I agree. There are two problems with adding overloads: 1. You may make existing calls ambiguous. This should be prevented as much as possible (by adding sufficiently many additional overloads to restore unambiguity). 2. You can make taking a function pointer ambiguous when adding the 2nd overload. This cannot be prevented, and we shouldn't strive to (effectively) ban overloads because of it. If examples and other docs would consistently use qOverload, instead of pretending that the naked addressof operator is sufficient, our users wouldn't hit the compiler errors mentioned by Ola. It should also be noted that there are two categories of SiCs: a. those that can be fixed client-side without breaking compat with older Qt versions, and b. those which cannot IMO, SiCs of type (a) are acceptable, those of type (b) are obviously no-no's. Adding an overload (both cases: 1 + 2 above) is type (a), so is acceptable. Adding explicit to a ctor that should have been but wasn't is also SiC Type (a). In these cases, code that breaks is code that deserves to be broken, because it was brittle. Renaming a public inline function of a non-exported class is BC, but SiC Type (b), so not acceptable. Thanks, Marc -- Marc Mutz | Senior Software Engineer KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - Qt, C++ and OpenGL Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
Em quarta-feira, 13 de julho de 2016, às 16:45:05 PDT, Ola Røer Thorsen escreveu: > In my opinion, please try to avoid these overloads as much as possible. That's the current policy. -- 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] Source break policy for function overloads
Em quarta-feira, 13 de julho de 2016, às 10:44:13 PDT, Alexander Blasche escreveu: > Hi, > > Yesterday, I stumbled over a break in QtSerialBus due to the addition of new > QTimer::setInterval(..) overloads in qtbase/dev. This was introduced by > > https://codereview.qt-project.org/#/c/160889/ > > Unfortunately this has the undesirable side effect that lines like: > > q->connect(q, &QModbusClient::timeoutChanged, element.timer.data(), > &QTimer::setInterval); > > do not compile anymore. The function pointer to setInterval() becomes > ambiguous. While the fix (https://codereview.qt-project.org/#/c/165034/) > was not difficult once you know what is happening this can hit every > customer and basically reduces the source compatibility promise into > absurdity. > > While talking to several devs in the office I could not find a congruent > opinion on this issue. Do we or even can we care? Do we have a policy? We do have this policy since 5.0 that we have to be careful about overloads due to the connect syntax. The breakage was accidental and not intended. I did intend to add overloads, as they're nice to use, I just hadn't considered the consequences. -- 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] Source break policy for function overloads
2016-07-13 16:30 GMT+02:00 Simon Hausmann : > > It all comes down to the question: What impression do we want to give > people when they upgrade to > > a newer version of Qt? > > > In my experience, I first had problems with this when I converted to the new "connect" syntax some years ago and hit the overloaded "error" functions in QAbstractSocket where just one of them is a signal. It cost me quite some time figuring out how to solve that, ending with getting help on the interest mailing list. If qOverload would have existed back then, I would not have known that I should or could use that either. So if similar problems appear when upgrading Qt because of added functionality it will cost developers a lot of time. The compiler error will probably not point to a suggested solution like "use qOverload" or "use static_cast(&QTcpSocket::error)" instead. In my opinion, please try to avoid these overloads as much as possible. Cheers, Ola ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
Hi, I suppose that if this is the first time an overload is added, then the risk of ambiguity exists. It seems that this is the case here. If released API has overloads and a new overload is added, then the caller code using QObject::connect would already use qOverload or similar, right? In that case perhaps it is not necessary to document the addition as source compatibility may be preserved. It all comes down to the question: What impression do we want to give people when they upgrade to a newer version of Qt? Simon From: Development on behalf of Giuseppe D'Angelo Sent: Wednesday, July 13, 2016 4:16:39 PM To: development@qt-project.org Subject: Re: [Development] Source break policy for function overloads Il 13/07/2016 15:55, Simon Hausmann ha scritto: > > I think if we allow source compatibility breakages like the one here > (overloaded slot added), then I think > > it should require a prominent notice in the changelog / release notes. Note that setInterval is not even a slot (but an overload of start(), a slot, was also added in the same commit). Do we need to add changelog notes for *any* overload added to free functions or QObject subclass member functions? Cheers, -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
Il 13/07/2016 15:55, Simon Hausmann ha scritto: I think if we allow source compatibility breakages like the one here (overloaded slot added), then I think it should require a prominent notice in the changelog / release notes. Note that setInterval is not even a slot (but an overload of start(), a slot, was also added in the same commit). Do we need to add changelog notes for *any* overload added to free functions or QObject subclass member functions? Cheers, -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908 KDAB - The Qt Experts smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
Hi, I think in the interest of our users and the popularity of Qt, the maintenance of source compatibility should be a supremely important goal. That said, breakages of source compatibility for the sake of cleanups appear to have been accepted by approvers in the past. I think if we allow source compatibility breakages like the one here (overloaded slot added), then I think it should require a prominent notice in the changelog / release notes. One can argue that with includes it's pretty easy to "fix" the error, but with the QObject::connect error on ambiguous overloads the error messages are crap and I think we should make an effort help our users. Simon From: Development on behalf of Jędrzej Nowacki Sent: Wednesday, July 13, 2016 1:39:19 PM To: development@qt-project.org Subject: Re: [Development] Source break policy for function overloads On Wednesday 13 of July 2016 10:44:13 Alexander Blasche wrote: > Hi, > > Yesterday, I stumbled over a break in QtSerialBus due to the addition of new > QTimer::setInterval(..) overloads in qtbase/dev. This was introduced by > > https://codereview.qt-project.org/#/c/160889/ > > Unfortunately this has the undesirable side effect that lines like: > > q->connect(q, &QModbusClient::timeoutChanged, element.timer.data(), > &QTimer::setInterval); > > do not compile anymore. The function pointer to setInterval() becomes > ambiguous. While the fix (https://codereview.qt-project.org/#/c/165034/) > was not difficult once you know what is happening this can hit every > customer and basically reduces the source compatibility promise into > absurdity. > > While talking to several devs in the office I could not find a congruent > opinion on this issue. Do we or even can we care? Do we have a policy? > > -- > Alex > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development Hi, We do not have SC promise only BC. We tend to not break SC without reason, for example we avoid juggling with header files just to "cleanup" stuff, but definitely we reserve right to add overloads and new functions. That is the current state, whatever it make sense or not is a different discussion. Cheers, Jędrek ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
Il 13/07/2016 12:44, Alexander Blasche ha scritto: While talking to several devs in the office I could not find a congruent opinion on this issue. Do we or even can we care? Do we have a policy? While it would be nice not to add overloads to signals and slots, and we can keep an eye on that, the fact that you can connect to *any* function makes such a policy hard to enforce (unless we start introducing a weird naming policy for functions). Note that this has nothing to do with Qt, but it's a C++ language "feature" -- hence I would say that if you care about your source compatibility you should start using qOverload in your code base every time you take a pointer to a function. My 2 cents, -- Giuseppe D'Angelo | giuseppe.dang...@kdab.com | Senior Software Engineer KDAB (UK) Ltd., a KDAB Group company | Tel: UK +44-1625-809908 KDAB - The Qt Experts smime.p7s Description: Firma crittografica S/MIME ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
> -Original Message- > From: Development [mailto:development-bounces+kai.koehne=qt.io@qt- > [...] > We do not have SC promise only BC. We tend to not break SC without > reason, for example we avoid juggling with header files just to "cleanup" > stuff, but definitely we reserve right to add overloads and new functions. > That is the current state, whatever it make sense or not is a different > discussion. We should fix https://wiki.qt.io/Qt-Version-Compatibility then. I agree though that we can't guarantee source compatibility in all cases (indirect includes are one example). We could establish a rule that methods marked with Q_SLOTS shouldn't be overloaded. Not that this would've helped, since QTimer::setInterval is not marked as a slot. Regards Kai ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Source break policy for function overloads
On Wednesday 13 of July 2016 10:44:13 Alexander Blasche wrote: > Hi, > > Yesterday, I stumbled over a break in QtSerialBus due to the addition of new > QTimer::setInterval(..) overloads in qtbase/dev. This was introduced by > > https://codereview.qt-project.org/#/c/160889/ > > Unfortunately this has the undesirable side effect that lines like: > > q->connect(q, &QModbusClient::timeoutChanged, element.timer.data(), > &QTimer::setInterval); > > do not compile anymore. The function pointer to setInterval() becomes > ambiguous. While the fix (https://codereview.qt-project.org/#/c/165034/) > was not difficult once you know what is happening this can hit every > customer and basically reduces the source compatibility promise into > absurdity. > > While talking to several devs in the office I could not find a congruent > opinion on this issue. Do we or even can we care? Do we have a policy? > > -- > Alex > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development Hi, We do not have SC promise only BC. We tend to not break SC without reason, for example we avoid juggling with header files just to "cleanup" stuff, but definitely we reserve right to add overloads and new functions. That is the current state, whatever it make sense or not is a different discussion. Cheers, Jędrek ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
