From kde-frameworks-devel Sat Feb 22 09:03:08 2014 From: "David Faure" Date: Sat, 22 Feb 2014 09:03:08 +0000 To: kde-frameworks-devel Subject: Re: Review Request 115028: Inline deprecated KUser::fullName() method Message-Id: <20140222090308.7229.38868 () probe ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-frameworks-devel&m=139305980710499 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============1831752241877366219==" --===============1831752241877366219== Content-Type: multipart/alternative; boundary="===============5152265774715031482==" --===============5152265774715031482== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit > On Feb. 20, 2014, 12:16 p.m., Kevin Ottens wrote: > > src/lib/CMakeLists.txt, line 86 > > > > > > Not sure about that... I don't think we want the library itself to use deprecated methods. So knowing about that through warnings would make sense. > > Alex Merry wrote: > This actually ends up making no difference for KCoreAddons. Other frameworks have annoying false positives where there are deprecated slots, as these are used from the moc-generated code. And also tests for deprecated methods. > > Note that this would not silence warnings about using deprecated code from other libraries. If it makes no difference for kcoreaddons, let's leave deprecation warnings enabled (in case we deprecate something else later). But indeed in the frameworks where it's not fixable (deprecated slots), we could add such a line, with a comment about why. False positives are annoying and can make people ignore real issues. Unittest for deprecated code... maybe we can add the definition (to disable warnings) *just* for these tests, after splitting them out... - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115028/#review50343 ----------------------------------------------------------- On Feb. 6, 2014, 5:20 p.m., Alex Merry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/115028/ > ----------------------------------------------------------- > > (Updated Feb. 6, 2014, 5:20 p.m.) > > > Review request for KDE Frameworks. > > > Repository: kcoreaddons > > > Description > ------- > > Inline deprecated KUser::fullName() method > > > Use KCOREADDONS_NO_DEPRECATED instead of KDE_NO_DEPRECATED > > KCOREADDONS_NO_DEPRECATED is the macro controlled by > generate_export_header; KDE_NO_DEPRECATED is left over from kdelibs. > > Disable deprecation macro when building the library itself > > This prevents spurious compiler warnings (particularly when slots are > deprecated). > > > Diffs > ----- > > src/lib/util/kuser.h 2b6e6ed92bc1465945f36f2fde821f36fa51585f > src/lib/util/kuser_unix.cpp 8a3a39d379ca863b4906bb01228c5e01a5b955b0 > src/lib/util/kuser_win.cpp 6a6cbb1751bd569d8684f8e11add1ef304c0a94d > src/lib/CMakeLists.txt e48904dabe7b2790599c34673832b6ce38eab0e3 > > Diff: https://git.reviewboard.kde.org/r/115028/diff/ > > > Testing > ------- > > configures, compiles, tests pass (well, except KDirWatch-FAM, which has never passed for me). > > > Thanks, > > Alex Merry > > --===============5152265774715031482== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit

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

On February 20th, 2014, 12:16 p.m. UTC, Kevin Ottens wrote:

src/lib/CMakeLists.txt (Diff revision 2)
		86	target_compile_definitions(KF5CoreAddons PRIVATE KCOREADDONS_DEPRECATED=)

Not sure about that... I don't think we want the library itself to use deprecated methods. So knowing about that through warnings would make sense.

On February 20th, 2014, 12:54 p.m. UTC, Alex Merry wrote:

This actually ends up making no difference for KCoreAddons.  Other frameworks have annoying false positives where there are deprecated slots, as these are used from the moc-generated code.  And also tests for deprecated methods.

Note that this would not silence warnings about using deprecated code from other libraries.

If it makes no difference for kcoreaddons, let's leave deprecation warnings enabled (in case we deprecate something else later).
But indeed in the frameworks where it's not fixable (deprecated slots), we could add such a line, with a comment about why.
False positives are annoying and can make people ignore real issues.

Unittest for deprecated code... maybe we can add the definition (to disable warnings) *just* for these tests, after splitting them out...

- David

On February 6th, 2014, 5:20 p.m. UTC, Alex Merry wrote:

Review request for KDE Frameworks.

By Alex Merry.

Updated Feb. 6, 2014, 5:20 p.m.

Repository: kcoreaddons

Description

Inline deprecated KUser::fullName() method


Use KCOREADDONS_NO_DEPRECATED instead of KDE_NO_DEPRECATED

KCOREADDONS_NO_DEPRECATED is the macro controlled by
generate_export_header; KDE_NO_DEPRECATED is left over from kdelibs.

Disable deprecation macro when building the library itself

This prevents spurious compiler warnings (particularly when slots are
deprecated).

Testing

configures, compiles, tests pass (well, except KDirWatch-FAM, which has never passed for me).

Diffs

src/lib/util/kuser.h (2b6e6ed92bc1465945f36f2fde821f36fa51585f)
src/lib/util/kuser_unix.cpp (8a3a39d379ca863b4906bb01228c5e01a5b955b0)
src/lib/util/kuser_win.cpp (6a6cbb1751bd569d8684f8e11add1ef304c0a94d)
src/lib/CMakeLists.txt (e48904dabe7b2790599c34673832b6ce38eab0e3)

--===============5152265774715031482==-- --===============1831752241877366219== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel --===============1831752241877366219==--