> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
