Refactor TribitsAdjustPackageEnables.cmake to get ready to transition to new data-structures (#63) #532
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is to improve understandability and make the next set of refactorings easier moving to complete TriBITSPub#63. The main things I did were: * Moved tribits_adjust_package_enables() to top ("Clean Code" order) * Factor out tribits_unenable_enabled_packages(), add test * Factor out tribits_sweep_forward_apply_disables() * Factor out tribits_sweep_forward_apply_enables() * Factor out tribits_disable_and_enable_tests_and_examples() * Factor out tribits_sweep_backward_enable_upstream_packages() * Factor out tribits_sweep_backward_enable_required_tpls() * Factor out tribits_set_cache_vars_for_current_enabled_packages() * Factor out tribits_enable_parent_package_for_subpackage_enable() * Factor out tribits_setup_direct_packages_dependencies_lists_and_lib_required_enable_vars() Other things I did as part of this refactoring: * I have started namespacing local vars in these macros to avoid collisions with variables above and below in the macro call stack (after discussion with @KyleFromKitware). * Upgrading foreach() to use foreach ( ... IN LISTS ... ) (Hopefully this will improve some performance but also seems the right thing to do.) * Remove leading and trailing message("") and instead use "\n" and beginning and end
This is using a form of "Clean Code" ordering where the top-level macros/functions are at the top and the macros/functions that they call are below them. NOTE: I purposefully did not update the foreach() loops involving the list vars `<Package>_[FORWARD_][LIB|TEST]_[OPTIONAL|REQUIRED]_DEP_[TPLS|PACKAGES` because all of those loops are going to be replaced with the new vars `<Package>_[LIB|TEST]_DEFINED_DEPENDENCIES` and `<Package>_[LIB|TEST]_DEP_REQUIRED_<depPkg>` in the next topic-branch.
bartlettroscoe
added
component: core
type: refactor
KyleFromKitware
requested changes
Oct 18, 2022
| (NOT ${PROJECT_NAME}_ENABLE_${PACKAGE_NAME}) | ||
| AND | ||
| (NOT "${${PROJECT_NAME}_ENABLE_${PACKAGE_NAME}}" STREQUAL "") | ||
| ) |
There was a problem hiding this comment.
The formatting of this if() statement is a little awkward. Any way you could make it a little nicer?
There was a problem hiding this comment.
@KyleFromKitware, what would you suggest for a multi-line if () statement in CMake?
For multi-line if () statement clauses in CMake, I have moved to the formatting:
if ( <if-clause-line1>
<if-clause-line2> # Indented 2 increments
...
<if-clause-lineN> # Indented 2 increments
) # Closing parentheses indented 1 increment to line up with execution statements
<execute-statement1>
<execute-statement2>
...
<execute-statementM>
endif()
If you know the style, it makes it easy to differentiate the different parts of the if statement and avoids too much right drift.
There was a problem hiding this comment.
For this particular if() statement, I would do something like the following:
if((NOT ${PROJECT_NAME}_ENABLE_${PACKAGE_NAME})
AND (NOT "${${PROJECT_NAME}_ENABLE_${PACKAGE_NAME}}" STREQUAL ""))There was a problem hiding this comment.
That is pretty close to what I did, but I really like having that closing ) on its own line, lined up with the execute statements. That makes it easy to see where the if clause ends and the if statements start (at least for me).
| # Set <ParentPackage>_ENABLE_<SubPackage>=ON if not already enabled for all | ||
| # subpackages of a parent package. | ||
| # | ||
| macro(tribits_postprocess_package_with_subpackages_optional_subpackage_enables | ||
| PACKAGE_NAME | ||
| ) | ||
| #message("TRIBITS_POSTPROCESS_PACKAGE_WITH_SUBPACKAGES_TEST_EXAMPLE_ENABLES '${PACKAGE_NAME}'") | ||
| foreach(TRIBITS_SUBPACKAGE ${${PACKAGE_NAME}_SUBPACKAGES}) | ||
| set(SUBPACKAGE_FULLNAME ${PACKAGE_NAME}${TRIBITS_SUBPACKAGE}) | ||
| foreach(tap3_subPkg ${${PACKAGE_NAME}_SUBPACKAGES}) |
| @@ -1200,8 +1324,8 @@ macro(tribits_postprocess_package_with_subpackages_test_example_enables | |||
| PACKAGE_NAME TESTS_OR_EXAMPLES | |||
| ) | |||
| #message("TRIBITS_POSTPROCESS_PACKAGE_WITH_SUBPACKAGES_TEST_EXAMPLE_ENABLES '${PACKAGE_NAME}'") | |||
| foreach(TRIBITS_SUBPACKAGE ${${PACKAGE_NAME}_SUBPACKAGES}) | |||
| set(SUBPACKAGE_FULLNAME ${PACKAGE_NAME}${TRIBITS_SUBPACKAGE}) | |||
| foreach(tap3_subPkg ${${PACKAGE_NAME}_SUBPACKAGES}) | |||
…Pub#532) * Reformatted multi-line if() statement according to my current prefered style. * Add IN LISTS to a few foreach() statements * Removed a bunch of commented out debug print statements
|
Hello @KyleFromKitware, please have a look at the changes in the new commit I am curious how you would suggest formatting multi-line if() statement clauses. |
KyleFromKitware
approved these changes
Oct 18, 2022
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is a refactoring of the module
TribitsAdjustPackageEnables.cmaketo make it easier to understand and navigate. This makes the code more maintainable and makes it easier to refactor to use the new combined TPL and internal package data-structures to complete #63.Instructions to Reviewers
IN LISTSin someforeach()loops (see the detailed commit log messages for more details).IN LISTSin theforeach()statements involving the list vars<Package>_[FORWARD_][LIB|TEST]_[OPTIONAL|REQUIRED]_DEP_[TPLS|PACKAGESbecause all of those loops are going to be replaced with the new vars<Package>_[LIB|TEST]_DEFINED_DEPENDENCIESand<Package>_[LIB|TEST]_DEP_REQUIRED_<depPkg>in the next PR.