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

Tue Jun 3 10:12:03 UTC 2014

-----------------------------------------------------------
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
<https://git.reviewboard.kde.org/r/118489/#comment41106>

    Since when? How to port away?

src/core/kconfig.h
<https://git.reviewboard.kde.org/r/118489/#comment41105>

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

src/core/kcoreconfigskeleton.h
<https://git.reviewboard.kde.org/r/118489/#comment41107>

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

src/core/kcoreconfigskeleton.h
<https://git.reviewboard.kde.org/r/118489/#comment41100>

    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
<https://git.reviewboard.kde.org/r/118489/#comment41102>

    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
<https://git.reviewboard.kde.org/r/118489/#comment41103>

    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
<https://git.reviewboard.kde.org/r/118489/#comment41104>

    What are you supposed to replace it with?

src/gui/kconfigloader.h
<https://git.reviewboard.kde.org/r/118489/#comment41111>

    @reimp?

src/gui/kconfigloader.h
<https://git.reviewboard.kde.org/r/118489/#comment41110>

    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 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/7f369eb3/attachment-0001.html>