Same as https://phabricator.kde.org/D22698 for ecm_qt_declare_logging_category.
Details
Details
Diff Detail
Diff Detail
- Repository
- R240 Extra CMake Modules
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
How about pass it as a TARGET argument using CMakeParseArguments.
You can see documentation about it here: https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html
+1 to the change, I really dislike using variables for this.
Comment Actions
I'm not sure if its beneficial to use cmake_parse_arguments here. It seems like it would make the macro harder to understand compared to the current change. I'm also not entirely sure how it would work? I haven't worked with cmake_parse_arguments before.
One thing that might cause issues with the current change is that passing a target that doesn't exist will not fail but instead just create a new list variable (although that's also the case without the change since you can pass wrong list variables to the macro and it wouldn't notice).
| modules/ECMQtDeclareLoggingCategory.cmake | ||
|---|---|---|
| 133 | -1 for changing the minimum CMake version in a macro. The change must be compatible the project requirement. | |
| modules/ECMQtDeclareLoggingCategory.cmake | ||
|---|---|---|
| 133 | must be compatible with the project.. | |
| modules/ECMQtDeclareLoggingCategory.cmake | ||
|---|---|---|
| 133 | What's the alternative for extra-cmake-modules? Can I just remove the cmake_minimum_required call or do I need to update the version elsewhere? | |
| 133 | I found that the CMake script of extra-cmake-modules itself specifies 2.8.12 as the minimum so I'm assuming that's the minimum version for the modules as well. The advantage of specifying cmake_minimum_required inline here is that cmake-extra-modules will continue to work with versions of CMake older than 3.1 and give a better error message when the new functionality is used with an older CMake version. Of course, if its not a problem to update the minimum required version to 3.1 that's the preferred solution compared to specifying cmake_minimum_required inline. | |
| modules/ECMQtDeclareLoggingCategory.cmake | ||
|---|---|---|
| 133 | If the change cannot be made compatible with the minimum ECM version, I suggest waiting for KF6. | |
Comment Actions
I
| modules/ECMQtDeclareLoggingCategory.cmake | ||
|---|---|---|
| 133 | Personally, I don't see the harm in adding a completely backwards compatible optional feature that requires a higher CMake version (that's already considered very old) but of course it's up to you guys to make the call. If it is not accepted, can I just leave the revision open until it can be merged for KF6? | |