feat(cmake): Enhance cpp_library() with argument validation, test-only linking, and improved docstrings.
#44
Conversation
WalkthroughThis change introduces a new CMake function, Changes
Sequence Diagram(s)sequenceDiagram
participant User as CMake Config
participant CLib as cpp_library
participant Validator as require_argument_values
User->>CLib: Call cpp_library(args)
CLib->>Validator: Validate required arguments
alt Missing argument values
Validator-->>CLib: Return fatal error
CLib-->>User: Configuration error reported
else All required values present
CLib->>CLib: Process updated linking logic (interface, non-interface, test linking)
CLib-->>User: Configuration continues successfully
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Bill-hbrhbr
changed the title
cpp_library(); Adding argument missing value validation; Fix linking with header-only libraries; Add TESTS_LINK_LIBRARIES for test-only tools; Improve doc strings with types.
Bill-hbrhbr
changed the title
cpp_library(); Adding argument missing value validation; Fix linking with header-only libraries; Add TESTS_LINK_LIBRARIES for test-only tools; Improve doc strings with types.cpp_library(); Adding argument missing value validation; Fix linking for header-only libraries; Add TESTS_LINK_LIBRARIES for test-only tools; Improve doc strings with types.
Bill-hbrhbr
changed the title
cpp_library(); Adding argument missing value validation; Fix linking for header-only libraries; Add TESTS_LINK_LIBRARIES for test-only tools; Improve doc strings with types.cpp_library(); Adding argument missing value validation; Fix linking for header-only libraries; Add TESTS_LINK_LIBRARIES for test-only tools; Improve doc strings with types.
Bill-hbrhbr
changed the title
cpp_library(); Adding argument missing value validation; Fix linking for header-only libraries; Add TESTS_LINK_LIBRARIES for test-only tools; Improve doc strings with types.cpp_library() with argument validation, test-only linking, and improved docstrings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CMake/ystdlib-cpp-helpers.cmake (2)
37-37: Fix documentation typoThere's a typo in the parameter documentation.
-# @parms {string[]} TESTS_SOURCES +# @param {string[]} TESTS_SOURCES
41-41: Fix documentation typoThere's a typo in the parameter documentation.
-# @parms {string[]} [TESTS_LINK_LIBRARIES] +# @param {string[]} [TESTS_LINK_LIBRARIES]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMake/ystdlib-cpp-helpers.cmake(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (7)
CMake/ystdlib-cpp-helpers.cmake (7)
1-10: Well-implemented validation functionThe new
require_argument_valuesfunction provides a clean, centralized way to validate required arguments, improving the fail-fast behavior when arguments are missing. The documentation clearly indicates parameter types and purpose.
57-59: Good addition of test-only linking supportAdding
TESTS_LINK_LIBRARIEStomultiValueArgssatisfies the PR objective of allowing third-party libraries that are necessary for testing but not required in the implementation of the target library. Also, movingBUILD_INCLUDE_DIRtomultiValueArgsis appropriate as it can accept multiple include paths.
60-65: Improved argument validationAdding
TESTS_SOURCEStorequireValueArgsensures proper validation when using the test functionality. This change aligns with the PR objective of enhancing argument validation.
71-75: Centralized argument validationReplacing conditional checks with a call to
require_argument_valuescentralizes the validation logic and makes the code more maintainable. This change aligns with the PR objective of enforcing argument validation in a fail-fast manner.
77-79: Simple BUILD_INCLUDE_DIR default handlingThis check correctly handles the default value for
BUILD_INCLUDE_DIRwhen not defined by the user. The replacement of the previous validation with the new centralized validation function maintains the same behavior while improving code organization.
135-135: Test-only linking properly implementedAdding the
TESTS_LINK_LIBRARIESto the unit test target's dependencies allows for test-only dependencies, satisfying a key PR objective.
148-153: Consistent linking for unified test targetThe implementation correctly applies the same test library linking to the unified test target, ensuring consistent behavior between individual and unified tests. This is a good improvement that maintains the symmetry between the two test targets.
davidlion
left a comment
davidlion
left a comment
There was a problem hiding this comment.
The suggestions should fit the character limit, but double check just in case.
Co-authored-by: davidlion <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CMake/ystdlib-cpp-helpers.cmake (1)
37-37: Correct the typo in parameter documentation.There's a typo in the parameter documentation -
@parmsshould be@param.-# @parms {string[]} TESTS_SOURCES +# @param {string[]} TESTS_SOURCES -# @parms {string[]} [TESTS_LINK_LIBRARIES] +# @param {string[]} [TESTS_LINK_LIBRARIES]Also applies to: 41-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMake/ystdlib-cpp-helpers.cmake(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (7)
CMake/ystdlib-cpp-helpers.cmake (7)
1-10: Excellent addition of a helper function for centralized argument validation.This new
require_argument_valuesfunction implements a fail-fast validation pattern for required arguments, which improves error reporting and makes the code more maintainable. The implementation is clear and the error messages are informative.
57-59: Good addition of TESTS_LINK_LIBRARIES to multiValueArgs.The addition of
TESTS_LINK_LIBRARIESto themultiValueArgslist allows for test-specific library dependencies, which is a useful feature for projects that need different libraries for testing compared to their main implementation.
60-65: Well-structured required argument validation.The explicit definition of
requireValueArgsmakes it clear which arguments must have values, improving code maintainability and documentation.
71-75: Great implementation of centralized argument validation.The call to
require_argument_valuescentralizes argument validation logic, making the code more maintainable and consistent in error handling.
77-79: Improved conditional logic for BUILD_INCLUDE_DIR.This code handles the default value assignment for
BUILD_INCLUDE_DIRafter validation, which is a cleaner approach than the previous implementation.
135-135: Great addition for test-specific dependencies.Adding
${arg_cpp_lib_TESTS_LINK_LIBRARIES}to the unit test target's linked libraries fulfills the PR goal of allowing test-only library dependencies.
148-153: Consistent linking approach for unified unit test target.The updated structure for linking libraries to the unified unit test target matches the pattern used for individual test targets, providing consistency and clarity.
2 participants
Description
This PR refactors and improves the
cpp_library()CMake function:require_argument_values()helper to enforce argument check in a fail-fast way.TESTS_LINK_LIBRARIESfor 3rdparty libraries that are not used in implementation but are used for testing purposes for a target library.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit