Fix deprecation syntax in ktcpsocket.h
ClosedPublic

Authored by dfaure on Nov 19 2019, 1:17 PM.

Details

Summary

gcc (7.4.1 here) says:
ktcpsocket.h:171:22: warning: attribute ignored in declaration of ‘class KTcpSocket’ [-Wattributes]
class KIOCORE_EXPORT KTcpSocket: public QIODevice

^~~~~~~~~~

ktcpsocket.h:171:22: note: attribute for ‘class KTcpSocket’ must follow the ‘class’ keyword

Test Plan

builds without that warning

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18946
Build 18964: arc lint + arc unit
dfaure created this revision.Nov 19 2019, 1:17 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 19 2019, 1:17 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Nov 19 2019, 1:17 PM
kossebau added a comment.EditedNov 19 2019, 1:29 PM

One thing I saw with the Qt deprecation tags is that deprecation attributes were only added to constructor calls, not the class itself.
Which to me made some sense, as one should be warned when creating instances of that class. But once you are passed an instance due to other reasons, being warned about furher calls on the instance makes no real sense, as one has to use the instance now that it exists.
In the initial set of patches to KF with the new deprecation macros I also did it like that for deprecated classes, added the warning macros only to constructor or other generation functions, not next to the class keyword.

Not sure how compilers actually react on deprecation attributes to the class only, still have to look up in what warnings on which class usages that results.

Edit: So, this is one thing where I am unsure about best practice yet, so also not sure if not only the order should be changed here, but rather the deprecation warning macros be moved to constructor calls only.

As a data point: this commit changes things for kimap, which has code saying

src/imapstreamparser.cpp:493:            } else if (KTcpSocket *socket = qobject_cast<KTcpSocket *>(m_socket)) {
src/imapstreamparser.cpp-494-                qWarning() << "No incoming packet for" << dt.elapsed()/1000 << "seconds on TCP socket. state=" << socket->state() << "error=" << socket->error() << socket->errorString();

With the class being deprecated, this code now fails to build (because of the "-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x060000" big hammer).
The obvious fix is then to add this to kimap's CMakeLists.txt:

add_definitions(-DKIOCORE_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054000) # We need KTcpSocket

IMHO this is all correct. We *are* using a deprecated class, it's important to know it, even if we didn't instantiate it ourselves. Because this means there will be porting effort when the class is removed.
In some cases one can right away port to a non-deprecated solution, or in this case where we do need to keep support for older KF5 versions, we need to enable the use of the deprecated class for a little while longer.

As a data point: this commit changes things for kimap, which has code saying

src/imapstreamparser.cpp:493:            } else if (KTcpSocket *socket = qobject_cast<KTcpSocket *>(m_socket)) {
src/imapstreamparser.cpp-494-                qWarning() << "No incoming packet for" << dt.elapsed()/1000 << "seconds on TCP socket. state=" << socket->state() << "error=" << socket->error() << socket->errorString();

With the class being deprecated, this code now fails to build (because of the "-DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x060000" big hammer).
The obvious fix is then to add this to kimap's CMakeLists.txt:

add_definitions(-DKIOCORE_DISABLE_DEPRECATED_BEFORE_AND_AT=0x054000) # We need KTcpSocket

IMHO this is all correct. We *are* using a deprecated class, it's important to know it, even if we didn't instantiate it ourselves. Because this means there will be porting effort when the class is removed.
In some cases one can right away port to a non-deprecated solution, or in this case where we do need to keep support for older KF5 versions, we need to enable the use of the deprecated class for a little while longer.

I don't see KTcpSocket in kimap? Or is that some older branch? Then that most definitely should not have the 6.0 deprecation version set.

Oops, local patch! (to add some debugging code in case of timeout).

OK, that brown-paper-bag issue aside, the reasoning about deprecating classes remains :-)

vkrause accepted this revision.Nov 21 2019, 4:42 PM
This revision is now accepted and ready to land.Nov 21 2019, 4:42 PM
dfaure closed this revision.Nov 21 2019, 8:51 PM