Re: Review Request 118489: Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

[Alex Merry]

> On June 3, 2014, 10:12 a.m., Alex Merry wrote:
> > Changing the macro name is a no-brainer. Comments below; a lot of them are 
> > about binary compatibility issues, and these depend on how we expect this 
> > macro to be used. I believe that Qt5 makes all its deprecated functions 
> > header-only so that compiling Qt without deprecated symbols produces a 
> > version BC with the version with deprecated symbols, and you can also 
> > safely set the macro yourself in application code to hide all Qt's 
> > deprecated symbols without messing anything up.
> > 
> > As you've currently got it, the only way this macro is useful is to produce 
> > a separate, binary-incompatible version of the framework that has no 
> > deprecated code or symbols. And maybe that's the only thing it's worth 
> > supporting.
> > 
> > I also highlighted some of the @deprecated apidox things - they really need 
> > to be more helpful, and this seemed to be an "all the deprecated stuff" 
> > review.
> 
> Matthew Dawson wrote:
> Regarding the binary compatibility, kdelibs4support does a similar thing, 
> which is why I thought it might be appropriate, but I also published the RR 
> to be sure :) .  Keeping with my don't rock the boat policy, I'll take 
> whatever the Frameworks community deems to be appropriate.  We should decide 
> on a policy for deprecation in frameworks, and then publish that somewhere (I 
> may have a new volunteer that would even be willing to write it!).
> 
> Regarding the @deprecated apidox, I'll try to write something useful for 
> them and update the patch later tonight.  I won't comment on each one 
> invidivually.  Do you happen to have a good example of a @deprecated comment?

Yeah, I think that policy thing is a good idea. If you have a volunteer for 
that, that's great! The most productive approach is probably to draft something 
and send it to the list for feedback.

Example:
@deprecated since 5.0, use SomeClass::someMethod() instead


> On June 3, 2014, 10:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.h, lines 1211-1220
> > 
> >
> > This wrapping of the apidox in the deprecated macro has been done 
> > inconsistently.
> 
> Matthew Dawson wrote:
> Is there a standard for how Frameworks code should wrap the declarations? 
>  I'll fix any to be consistent with the wider codebase.

I don't think there's a standard, no. However, I think it makes sense to wrap 
the docs - that way, it should be possible to tell doxygen to assume 
KCONFIGCORE_NO_DEPRECATED is set, and it won't get confused by dox that aren't 
attached to a declaration.


> On June 3, 2014, 10:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.h, lines 1475-1487
> > 
> >
> > I think this is a bad idea. Binary compatibility is very important in 
> > Frameworks, and this breaks it. And messing with virtuals is not just BIC, 
> > it's BIC in a way that can cause weird error messages (rather than just 
> > linking failures).
> 
> Matthew Dawson wrote:
> As mentioned above, I wasn't sure what the policy should be.  I'm happy 
> doing it either way, but I think the wider community should decide.

Can I suggest sending a separate mail to the list? People tend to brush over 
RRs, especially ones someone else has already commented on.


- Alex


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118489/#review59060
---


On June 3, 2014, 6:34 a.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118489/
> ---
> 
> (Updated June 3, 2014, 6:34 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.
> 
> Inline with the new defines used by Frameworks, remove the KDE_NO_DEPRECATED.
> 
> Also remove some deprecated virtuals when compiling without deprecated 
> features.
> 
> Make sure there are no deprecated features left when deprecated features are
> compiled out.
> 
> When compiling without deprecated features, make sure not to use them.
> 
> KEmailSettings was using deprecated entries when the enumerations for the
> features were compiled out, which would fail to compile.  Thus, when compiling
> without deprecated features don't read extra configuration entries.
> 
> Remove usages of deprecated virtuals, instead use their replacement.
> 
> Go through and rename uses of usrWriteConfig to usrSave.  The change should
> invoke no behavioural differences.  Note that the kconfig

Re: Review Request 118489: Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

[Matthew Dawson]

> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > Changing the macro name is a no-brainer. Comments below; a lot of them are 
> > about binary compatibility issues, and these depend on how we expect this 
> > macro to be used. I believe that Qt5 makes all its deprecated functions 
> > header-only so that compiling Qt without deprecated symbols produces a 
> > version BC with the version with deprecated symbols, and you can also 
> > safely set the macro yourself in application code to hide all Qt's 
> > deprecated symbols without messing anything up.
> > 
> > As you've currently got it, the only way this macro is useful is to produce 
> > a separate, binary-incompatible version of the framework that has no 
> > deprecated code or symbols. And maybe that's the only thing it's worth 
> > supporting.
> > 
> > I also highlighted some of the @deprecated apidox things - they really need 
> > to be more helpful, and this seemed to be an "all the deprecated stuff" 
> > review.

Regarding the binary compatibility, kdelibs4support does a similar thing, which 
is why I thought it might be appropriate, but I also published the RR to be 
sure :) .  Keeping with my don't rock the boat policy, I'll take whatever the 
Frameworks community deems to be appropriate.  We should decide on a policy for 
deprecation in frameworks, and then publish that somewhere (I may have a new 
volunteer that would even be willing to write it!).

Regarding the @deprecated apidox, I'll try to write something useful for them 
and update the patch later tonight.  I won't comment on each one invidivually.  
Do you happen to have a good example of a @deprecated comment?


> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.h, lines 1211-1220
> > 
> >
> > This wrapping of the apidox in the deprecated macro has been done 
> > inconsistently.

Is there a standard for how Frameworks code should wrap the declarations?  I'll 
fix any to be consistent with the wider codebase.


> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.h, lines 1475-1487
> > 
> >
> > I think this is a bad idea. Binary compatibility is very important in 
> > Frameworks, and this breaks it. And messing with virtuals is not just BIC, 
> > it's BIC in a way that can cause weird error messages (rather than just 
> > linking failures).

As mentioned above, I wasn't sure what the policy should be.  I'm happy doing 
it either way, but I think the wider community should decide.


> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > src/core/kcoreconfigskeleton.cpp, lines 1271-1280
> > 
> >
> > I think this (and some of the other trivial deprecated methods) could 
> > be reasonably made header-only. This (a) makes versions of KConfig compiled 
> > with and without deprecated symbols more BC, and (b) documents the 
> > replacement code right there in the header.

Good point, I'll move such code into the header.


> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > src/gui/kconfigloader.h, line 168
> > 
> >
> > @reimp?

Will add.


> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > src/gui/kconfigloader.h, line 170
> > 
> >
> > Q_DECL_OVERRIDE?

Will add.


- Matthew


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118489/#review59060
---


On June 3, 2014, 2:34 a.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118489/
> ---
> 
> (Updated June 3, 2014, 2:34 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.
> 
> Inline with the new defines used by Frameworks, remove the KDE_NO_DEPRECATED.
> 
> Also remove some deprecated virtuals when compiling without deprecated 
> features.
> 
> Make sure there are no deprecated features left when deprecated features are
> compiled out.
> 
> When compiling without deprecated features, make sure not to use them.
> 
> KEmailSettings was using deprecated entries when the enumerations for the
> features were compiled out, which would fail to compile.  Thus, when compiling
> without deprecated features don't read extra configuration entries.
> 
> Remove usages of deprecated virtuals, instead use their 

Re: Review Request 118489: Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

[Alex Merry]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118489/#review59060
---


Changing the macro name is a no-brainer. Comments below; a lot of them are 
about binary compatibility issues, and these depend on how we expect this macro 
to be used. I believe that Qt5 makes all its deprecated functions header-only 
so that compiling Qt without deprecated symbols produces a version BC with the 
version with deprecated symbols, and you can also safely set the macro yourself 
in application code to hide all Qt's deprecated symbols without messing 
anything up.

As you've currently got it, the only way this macro is useful is to produce a 
separate, binary-incompatible version of the framework that has no deprecated 
code or symbols. And maybe that's the only thing it's worth supporting.

I also highlighted some of the @deprecated apidox things - they really need to 
be more helpful, and this seemed to be an "all the deprecated stuff" review.


src/core/kconfig.h


Since when? How to port away?



src/core/kconfig.h


Duplicate @deprecated, and neither of them say since when or give any 
indication of how to port away from them.



src/core/kcoreconfigskeleton.h


This wrapping of the apidox in the deprecated macro has been done 
inconsistently.



src/core/kcoreconfigskeleton.h


I think this is a bad idea. Binary compatibility is very important in 
Frameworks, and this breaks it. And messing with virtuals is not just BIC, it's 
BIC in a way that can cause weird error messages (rather than just linking 
failures).



src/core/kcoreconfigskeleton.cpp


I think this (and some of the other trivial deprecated methods) could be 
reasonably made header-only. This (a) makes versions of KConfig compiled with 
and without deprecated symbols more BC, and (b) documents the replacement code 
right there in the header.



src/core/kemailsettings.h


I would force the value of this, so that it is the same whether or not 
KCONFIGCORE_NO_DEPRECATED is set.



src/core/kemailsettings.h


What are you supposed to replace it with?



src/gui/kconfigloader.h


@reimp?



src/gui/kconfigloader.h


Q_DECL_OVERRIDE?


- Alex Merry


On June 3, 2014, 6:34 a.m., Matthew Dawson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118489/
> ---
> 
> (Updated June 3, 2014, 6:34 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kconfig
> 
> 
> Description
> ---
> 
> Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.
> 
> Inline with the new defines used by Frameworks, remove the KDE_NO_DEPRECATED.
> 
> Also remove some deprecated virtuals when compiling without deprecated 
> features.
> 
> Make sure there are no deprecated features left when deprecated features are
> compiled out.
> 
> When compiling without deprecated features, make sure not to use them.
> 
> KEmailSettings was using deprecated entries when the enumerations for the
> features were compiled out, which would fail to compile.  Thus, when compiling
> without deprecated features don't read extra configuration entries.
> 
> Remove usages of deprecated virtuals, instead use their replacement.
> 
> Go through and rename uses of usrWriteConfig to usrSave.  The change should
> invoke no behavioural differences.  Note that the kconfig_compiler has
> changed to the new function as well.
> 
> 
> Note that removing the deprecated virtuals does cause the non-deprecated 
> headers and deprecated libraries to be binary incompatible.  Since you can't 
> really do this I don't think its an issue.  Also, kdelibs4support (others may 
> too, haven't checked) uses a similar pattern. 
> 
> 
> Diffs
> -
> 
>   autotests/kconfig_compiler/test_signal.cpp.ref 
> 58e73efd77614edc4a5bd54bc06fbc34ccff2342 
>   autotests/kconfig_compiler/test_signal.h.ref 
> 19b8b4005e543e9e660f3ea016853c7a689ac17d 
>   autotests/kconfigtest.cpp 2b6de0d7d63df6aee69210aa09418628f0b8110a 
>   src/core/kconfig.h d27eebe7c41cb433b1808882c53cbf7b4c870950 
>   src/core/kconfig.cpp ea9746c001e235529a1cdd5865b9e1b5c129b56a 
>   src/core/kconfiggroup.h 3c4bce8433e3c5d4cb2d9fdd111a43f04cf3c295 
>   src/core/kconfiggroup.cpp 6f609b

Review Request 118489: Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

[Matthew Dawson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118489/
---

Review request for KDE Frameworks.


Repository: kconfig


Description
---

Change all occurrences of KDE_NO_DEPRECATED to an appropriate define.

Inline with the new defines used by Frameworks, remove the KDE_NO_DEPRECATED.

Also remove some deprecated virtuals when compiling without deprecated features.

Make sure there are no deprecated features left when deprecated features are
compiled out.

When compiling without deprecated features, make sure not to use them.

KEmailSettings was using deprecated entries when the enumerations for the
features were compiled out, which would fail to compile.  Thus, when compiling
without deprecated features don't read extra configuration entries.

Remove usages of deprecated virtuals, instead use their replacement.

Go through and rename uses of usrWriteConfig to usrSave.  The change should
invoke no behavioural differences.  Note that the kconfig_compiler has
changed to the new function as well.


Note that removing the deprecated virtuals does cause the non-deprecated 
headers and deprecated libraries to be binary incompatible.  Since you can't 
really do this I don't think its an issue.  Also, kdelibs4support (others may 
too, haven't checked) uses a similar pattern. 


Diffs
-

  autotests/kconfig_compiler/test_signal.cpp.ref 
58e73efd77614edc4a5bd54bc06fbc34ccff2342 
  autotests/kconfig_compiler/test_signal.h.ref 
19b8b4005e543e9e660f3ea016853c7a689ac17d 
  autotests/kconfigtest.cpp 2b6de0d7d63df6aee69210aa09418628f0b8110a 
  src/core/kconfig.h d27eebe7c41cb433b1808882c53cbf7b4c870950 
  src/core/kconfig.cpp ea9746c001e235529a1cdd5865b9e1b5c129b56a 
  src/core/kconfiggroup.h 3c4bce8433e3c5d4cb2d9fdd111a43f04cf3c295 
  src/core/kconfiggroup.cpp 6f609baefec5beaf38fdfedd6d192b395e3f8acb 
  src/core/kcoreconfigskeleton.h bb9c1cf936b87e2456726a2bb3428be42558b39f 
  src/core/kcoreconfigskeleton.cpp f9c9f26876922bc8dbed20d050b09f09868550ce 
  src/core/kemailsettings.h 03249e5006b309a39ed4b16f85b86439cbbe96a6 
  src/core/kemailsettings.cpp 230c2aa4ba1db85ae401e126de705cdbfd5a4a55 
  src/gui/kconfigloader.h 36eb182fbf1c241a043566a13d7c6c123a6e455f 
  src/gui/kconfigloader.cpp 1dd9e7fc44a367165fedc2e7760c8b524ecd210e 
  src/kconfig_compiler/kconfig_compiler.cpp 
28b151c579c2c40e118b3b738a1e6ac81e461b3e 

Diff: https://git.reviewboard.kde.org/r/118489/diff/


Testing
---

Unit tests still pass, regardless of whether or not deprecated features are 
enabled.


Thanks,

Matthew Dawson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel