D25401: Fix deprecation syntax in ktcpsocket.h

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25401

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25401: Fix deprecation syntax in ktcpsocket.h

[Volker Krause]

vkrause accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25401

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25401: Fix deprecation syntax in ktcpsocket.h

[David Faure]

dfaure added a comment.


  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 :-)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25401

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25401: Fix deprecation syntax in ktcpsocket.h

[Volker Krause]

vkrause added a comment.


  In D25401#564683 , @dfaure wrote:
  
  > As a data point: this commit changes things for kimap, which has code saying
  >
  >   src/imapstreamparser.cpp:493:} else if (KTcpSocket *socket = 
qobject_cast(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=0x06" 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25401

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25401: Fix deprecation syntax in ktcpsocket.h

[David Faure]

dfaure added a comment.


  As a data point: this commit changes things for kimap, which has code saying
  
src/imapstreamparser.cpp:493:} else if (KTcpSocket *socket = 
qobject_cast(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=0x06" 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25401

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25401: Fix deprecation syntax in ktcpsocket.h

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25401

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25401: Fix deprecation syntax in ktcpsocket.h

[David Faure]

dfaure created this revision.
dfaure added reviewers: vkrause, kossebau.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION 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

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25401

AFFECTED FILES
  src/core/ktcpsocket.h

To: dfaure, vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns