Conversation
Katze719
commented
Oct 26, 2025
Katze719
commented
- Introduced a function to check and add optional compiler warning flags if supported.
- Updated the CMakeLists.txt to conditionally collect and wrap these warnings for the ThreadSchedule target.
- Modified the affinity_for_node function in topology.hpp to ensure a default ThreadAffinity object is returned when no NUMA nodes are present, improving code clarity.
- Introduced a function to check and add optional compiler warning flags if supported. - Updated the CMakeLists.txt to conditionally collect and wrap these warnings for the ThreadSchedule target. - Modified the affinity_for_node function in topology.hpp to ensure a default ThreadAffinity object is returned when no NUMA nodes are present, improving code clarity.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for optional compiler warning flags (specifically -Wnrvo) and refactors the affinity_for_node function to improve NRVO (Named Return Value Optimization) eligibility. The changes ensure the compiler can better optimize return statements by using a single named variable throughout the function.
Key Changes:
- Added CMake infrastructure to conditionally check and apply optional compiler warning flags
- Refactored
affinity_for_nodeto declareThreadAffinityonce at function start for consistent NRVO behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| CMakeLists.txt | Added CMake function to check compiler flag support and conditionally enable -Wnrvo warning with proper generator expressions |
| include/threadschedule/topology.hpp | Moved ThreadAffinity declaration to function start and replaced empty brace initialization with named variable return |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| include(CheckCXXCompilerFlag) | ||
|
|
||
| function(ts_add_warning_if_supported flag out_var) | ||
| string(REPLACE "-" "_" _flag_var "${flag}") |
There was a problem hiding this comment.
The _flag_var variable name may conflict across multiple calls to this function with different flags. The variable should include a unique suffix or use a more specific naming pattern. Consider using CXX_SUPPORTS${_flag_var} or similar to ensure uniqueness when checking multiple flags.
| string(REPLACE "-" "_" _flag_var "${flag}") | |
| string(REPLACE "-" "_" _flag_var "${flag}") | |
| set(_flag_var "CXX_SUPPORTS${_flag_var}") |
| if(DEFINED ${out_var} AND NOT "${${out_var}}" STREQUAL "") | ||
| set(${out_var} "${${out_var}};${flag}" PARENT_SCOPE) | ||
| else() | ||
| set(${out_var} "${flag}" PARENT_SCOPE) | ||
| endif() |
There was a problem hiding this comment.
The manual string concatenation with semicolons can be replaced with CMake's list(APPEND ...) command for better maintainability and clarity. Replace lines 170-174 with: list(APPEND ${out_var} \"${flag}\") followed by set(${out_var} \"${${out_var}}\" PARENT_SCOPE).
| if(DEFINED ${out_var} AND NOT "${${out_var}}" STREQUAL "") | |
| set(${out_var} "${${out_var}};${flag}" PARENT_SCOPE) | |
| else() | |
| set(${out_var} "${flag}" PARENT_SCOPE) | |
| endif() | |
| list(APPEND ${out_var} "${flag}") | |
| set(${out_var} "${${out_var}}" PARENT_SCOPE) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| include(CheckCXXCompilerFlag) | ||
|
|
||
| function(ts_add_warning_if_supported flag out_var) | ||
| string(REPLACE "-" "_" _flag_var "${flag}") |
There was a problem hiding this comment.
The variable name _flag_var created by sanitizing the flag contains characters like 'W' and is used directly as the output variable for check_cxx_compiler_flag. However, on line 168, the sanitized string (e.g., '_Wnrvo') is used as a variable name without proper handling. CMake variable names from check_cxx_compiler_flag should be valid identifiers. The sanitized variable _flag_var should convert '-Wnrvo' to something like '_Wnrvo', but this may still contain uppercase letters at the start which could cause issues. Consider using a more robust sanitization that also handles the leading character, such as string(MAKE_C_IDENTIFIER \"CXX_FLAG_${flag}\" _flag_var) or a more explicit pattern like string(REGEX REPLACE \"[^a-zA-Z0-9_]\" \"_\" _flag_var \"HAS${flag}\").
| string(REPLACE "-" "_" _flag_var "${flag}") | |
| string(MAKE_C_IDENTIFIER "CXX_FLAG_${flag}" _flag_var) |
1 participant
