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

Wed Jun 4 09:42:50 UTC 2014

> 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
> > <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.
> 
> 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
> > <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).
> 
> 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_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/20140604/7c1725ae/attachment-0001.html>