D24892: Fix usage of the new deprecation macros for assignIconsToContextMenu

Fri Oct 25 19:25:22 BST 2019

kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.

  In D24892#553597 <https://phabricator.kde.org/D24892#553597>, @vkrause wrote:

  > In D24892#552838 <https://phabricator.kde.org/D24892#552838>, @kossebau wrote:
  >
  > > Looks good. Perfect would be if you tested KIconThemes  with EXCLUDE_DEPRECATED_BEFORE_AND_AT set to hexnumber(5.64.0), to see if there is no internal usage still happening, like with autotests which might need to support such a build with KICONTHEMES_ENABLE_DEPRECATED_SINCE (ENABLE variant, as external to lib).
  >
  >
  > Assuming I used this correctly (setting EXCLUDE_DEPRECATED_BEFORE_AND_AT to 054000)

  Hm, guess the "default:0" confuses people, EXCLUDE_DEPRECATED_BEFORE_AND_AT would be set as string in majpr.minor.patch format ("0" & "CURRENT" are special cases). That is documented on https://api.kde.org/ecm/module/ECMGenerateExportHeader.html but seemingly that is too far away, so will add to the option description text.

  > this doesn't build independent of assignIconsToContextMenu, due to the Q_ENUM code generated for KIconLoader still referencing the deprecated FileSystem enum element?

  Building with EXCLUDE_DEPRECATED_BEFORE_AND_AT set to 0, 4.8.0, 5.0.0 & CURRENT, to cover before & after versions, the build does not fail here.
  The generated Q_ENUM code should not see  FileSystem because (and yes, the actual code written is confusing, no idea yet for better macros (names) which do not twist the mind), because:
  when setting EXCLUDE_DEPRECATED_BEFORE_AND_AT, this results in both KICONTHEMES_ENABLE_DEPRECATED_SINCE & KICONTHEMES_BUILD_DEPRECATED_SINCE to disable code based on the value of EXCLUDE_DEPRECATED_BEFORE_AND_AT. The difference between ENABLE & BUILD is, that developers linking against the built lib can control the ENABLE ones using the *_DISABLE_DEPRECATED_BEFORE_AND_AT, but not the BUILD one, that is fixed to the value set for EXCLUDE_DEPRECATED_BEFORE_AND_AT at generation time of the library.

  Let's look at the code:

    #if KICONTHEMES_ENABLE_DEPRECATED_SINCE(4, 8)
            FileSystem,    ///< An icon that represents a file system. @deprecated Since 4.8. Use Place instead.
    #elif KICONTHEMES_BUILD_DEPRECATED_SINCE(4, 8)
            FileSystem_DEPRECATED_DO_NOT_USE,
    #endif

  When building KIconThemes, setting EXCLUDE_DEPRECATED_BEFORE_AND_AT to 4.8.0 or later, both conditions will not be met. So the compiler will skip any of enumerator variants. If setting to before 4.8.0, the first condition is already met during the build.

  When building against KIconThemes, setting _KICONTHEMES_DISABLE_DEPRECATED_BEFORE_AND_AT now controls what the compiler sees. _KICONTHEMES_DISABLE_DEPRECATED_BEFORE_AND_AT though will be forced by the generated export header code to be at least at the version of EXCLUDE_DEPRECATED_BEFORE_AND_AT.
  With _KICONTHEMES_DISABLE_DEPRECATED_BEFORE_AND_AT being set to 4.8.0 and later, the first condition is not met, so the actual enumerator name is not available. To keep the following enumerators at the correct auto numbers (or if in need still have an enumerator which though has the warning in its name), there then is the elif branch, which will be only reached if enabled in the build.

  Yes, very confusing by just looking at the raw code. Suggestions for better macro names (or better approaches) very welcome.

INLINE COMMENTS

> vkrause wrote in kicontheme.h:292
> I am unsure about that, therefore I'm opting for the safe approach :) It is IMHO possible some deprecate API will stay, e.g. if it got deprecated before having strict rules on porting away from such API or the porting impact turning out more complex than anticipated. I'm therefore only adding the KF6 todos in places where they could be executed immediately when disregarding BC.

Some API deprecated staying because porting away is not easy to do should be the exception though, no? 
I would rather flag only such API elements with a comment once it is found out in real world that porting is more hard than anticipated.
But at the point of time of deprecation, the whole idea of the warnings being inserted into the compiler is to have people stop using it, so it can be removed at the next occasion, no? Otherwise the whole warning is harmful noise making people ignore warnings.

And ideally the concept of deprecated-but-might-stay should be codified ideally in the macros, so the compiler can be taught to do what is wanted.

REPOSITORY
  R302 KIconThemes

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24892

To: vkrause, kossebau
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191025/c5aa6e8d/attachment.html>