Details
Diff Detail
- Repository
- R302 KIconThemes
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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).
In general, given even you missed this (both the build disabling of the implementation as well as testing the build with EXCLUDE_DEPRECATED_BEFORE_AND_AT, I asume), I wonder if the support for build-without-deprecated should not be removed again from KDE Frameworks (though not the ECM macro, it works and others might find it useful). KF contributors/builders might not really need it or gain from it before actually KF6 gets created, and things are fragile enough with the new deprecation macros even without that feature (think virtual methods & enums being accidentally wrapped by *_ENABLE_DEPRECATED_SINCE...
| src/kicontheme.h | ||
|---|---|---|
| 292 | Is "// TODO KF6 remove" really needed BTW? Personally I would be strict and for KF6 just dump any API deprecated in KF5 times. | |
Assuming I used this correctly (setting EXCLUDE_DEPRECATED_BEFORE_AND_AT to 054000) this doesn't build independent of assignIconsToContextMenu, due to the Q_ENUM code generated for KIconLoader still referencing the deprecated FileSystem enum element?
In general, given even you missed this (both the build disabling of the implementation as well as testing the build with EXCLUDE_DEPRECATED_BEFORE_AND_AT, I asume), I wonder if the support for build-without-deprecated should not be removed again from KDE Frameworks (though not the ECM macro, it works and others might find it useful). KF contributors/builders might not really need it or gain from it before actually KF6 gets created, and things are fragile enough with the new deprecation macros even without that feature (think virtual methods & enums being accidentally wrapped by *_ENABLE_DEPRECATED_SINCE...
It has just been added and I assume people still need to get used to it and we are still ironing out the last issues, so maybe give it a bit more time before pulling it out again right away :) It does have BC and maintenance challenges for sure, but IMHO that is ok as I don't really see it as something to use in production rather than as a tool to help with migrations to new major version.
| src/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. | |
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,
#endifWhen 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.
| src/kicontheme.h | ||
|---|---|---|
| 292 | Some API deprecated staying because porting away is not easy to do should be the exception though, no? 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. | |
"default:0" is not what confused me, but EXCLUDE_DEPRECATED_BEFORE_AND_AT set to hexnumber(5.64.0) in your original statement ;-)
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.
Doing a clean build and setting EXCLUDE_DEPREACTED_BEFORE_AND_AT at the *first* cmake run makes it work here reliably too.
So, to get back to the original question: yes, this patch fixes the build :)
| src/kicontheme.h | ||
|---|---|---|
| 292 | Going forward certainly yes. What I have doubts about in this regard are the things that got deprecated in the past, before we had clear guidelines for this. | |