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

Tue Jun 3 20:51:00 UTC 2014

> 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
> > <https://git.reviewboard.kde.org/r/118489/diff/1/?file=277673#file277673line1211>
> >
> >     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
> > <https://git.reviewboard.kde.org/r/118489/diff/1/?file=277673#file277673line1475>
> >
> >     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
> > <https://git.reviewboard.kde.org/r/118489/diff/1/?file=277674#file277674line1271>
> >
> >     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
> > <https://git.reviewboard.kde.org/r/118489/diff/1/?file=277677#file277677line168>
> >
> >     @reimp?

Will add.

> On June 3, 2014, 6:12 a.m., Alex Merry wrote:
> > src/gui/kconfigloader.h, line 170
> > <https://git.reviewboard.kde.org/r/118489/diff/1/?file=277677#file277677line170>
> >
> >     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 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140603/15493b68/attachment-0001.html>