Conversation
Replace ad-hoc benchmark registration with a declarative benchmark_suite / benchmark_runner framework that standardizes how benchmarks are defined, discovered, filtered, and reported across all three libraries (corosio, asio coroutine, asio callback). Suite/runner framework (perf/bench/common/suite.hpp): - benchmark_suite: declarative builder for grouping benchmarks by category with typed arguments, warmup hooks, and flags - benchmark_runner: drives suite execution with CLI filtering by library (--library), category (--category), and name (--bench) - bench::state: unified per-run state providing duration, elapsed time, ops/items/bytes counters, latency statistics, and custom counters - Per-suite library identity via add_suite(library, suite) — shown in run headers as (library) [category] name and as separate library/category/name fields in JSON output Benchmark output improvements: - Consistent Title Case formatting for custom counter labels - Column-aligned output with std::setw(15) - Integer-valued doubles printed without decimals - Duration displayed without space (3s not 3 s) Latency measurement: - All latency recording uses nanoseconds (elapsed_ns) including callback benchmarks that write directly to statistics objects - JSON metric suffixes changed from _us to _ns - format_latency() prints nanoseconds without auto-scaling Asio detection (CMakeLists.txt): - Three-tier detection: Boost super-project Asio, standalone find_package, FetchContent fallback from GitHub All 21 benchmark factory files simplified to return a benchmark_suite from a make_*_suite() function, eliminating per-file boilerplate for argument parsing, timing, and output.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes CMake dependency resolution and TLS/provider wiring via a new cmake/CorosioBuild.cmake (corosio_resolve_deps, corosio_find_tls_provider, corosio_install), removes per-target FetchContent/MRDOCS/TLS plumbing from top-level CMakeLists, and updates perf benchmarks to use suite-based instantiation and nanosecond timing. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer CMakeLists
participant CB as cmake/CorosioBuild.cmake
participant TLS as TLS Providers (wolfssl / openssl)
participant INST as Installer / Export generator
Dev->>CB: include(CorosioBuild)\ncorosio_resolve_deps()
CB->>CB: resolve Boost siblings, Capy (Find/FetchContent), Threads
Dev->>CB: corosio_find_tls_provider(name)
CB->>TLS: locate provider (FindPackage / existing targets)
TLS-->>CB: return provider targets, includes, libs
CB->>CB: corosio_add_tls_library(name)\ncreate boost_corosio_${name} + Boost::corosio_${name}
Dev->>CB: corosio_install()
CB->>INST: emit install rules, exports, CMake config/version files
INST-->>Dev: installed package (boost_corosio + TLS variants)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://177.corosio.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-26 12:55:51 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
perf/common/perf.hpp (1)
216-222: Consider retaining adaptive unit formatting for readability.The change to always output nanoseconds simplifies the unit assumption but can produce unwieldy output for larger latencies (e.g.,
1500000000.00 nsinstead of1.50 s). If the benchmark suite may encounter latencies spanning multiple orders of magnitude, preserving the adaptive formatting—while accepting nanoseconds as input—would improve human readability.♻️ Optional: Retain adaptive formatting with nanoseconds input
// Format latency in nanoseconds inline std::string format_latency(double nanoseconds) { std::ostringstream oss; - oss << std::fixed << std::setprecision(2) << nanoseconds << " ns"; + oss << std::fixed << std::setprecision(2); + + if (nanoseconds >= 1e9) + oss << (nanoseconds / 1e9) << " s"; + else if (nanoseconds >= 1e6) + oss << (nanoseconds / 1e6) << " ms"; + else if (nanoseconds >= 1e3) + oss << (nanoseconds / 1e3) << " us"; + else + oss << nanoseconds << " ns"; + return oss.str(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@perf/common/perf.hpp` around lines 216 - 222, The current format_latency(double nanoseconds) always prints "ns"; instead convert the nanoseconds argument into an appropriate unit (ns, us, ms, s, maybe min) based on magnitude and format the scaled value with std::ostringstream and std::setprecision(2) so outputs like 1.50 s or 1.50 ms are produced; update format_latency to choose the unit, divide nanoseconds by the correct factor (1, 1e3, 1e6, 1e9, ...) and append the matching unit string while preserving the existing formatting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/CorosioBuild.cmake`:
- Around line 49-63: The two execute_process calls that probe git (the one
setting _corosio_branch with RESULT_VARIABLE _git_result and the one populating
_capy_has_branch) can hang on flaky networks; add a short TIMEOUT value (e.g.,
TIMEOUT 10 or 30) to each execute_process invocation so configure won't block
indefinitely, keeping the existing OUTPUT_STRIP_TRAILING_WHITESPACE, ERROR_QUIET
and RESULT_VARIABLE (_git_result) semantics and leaving variables
_corosio_branch and _capy_has_branch unchanged.
- Around line 48-49: Replace the CMake 3.14-only cache check "if(NOT DEFINED
CACHE{BOOST_COROSIO_CAPY_TAG})" with a minimum-version-compatible check that
tests the variable name itself (e.g. "if(NOT DEFINED BOOST_COROSIO_CAPY_TAG)"),
then only compute and call set(BOOST_COROSIO_CAPY_TAG <value> CACHE STRING "..."
) inside that block so existing user-supplied cache values are preserved; update
the condition and the surrounding logic where BOOST_COROSIO_CAPY_TAG is set to
avoid using the CACHE{...} syntax.
---
Nitpick comments:
In `@perf/common/perf.hpp`:
- Around line 216-222: The current format_latency(double nanoseconds) always
prints "ns"; instead convert the nanoseconds argument into an appropriate unit
(ns, us, ms, s, maybe min) based on magnitude and format the scaled value with
std::ostringstream and std::setprecision(2) so outputs like 1.50 s or 1.50 ms
are produced; update format_latency to choose the unit, divide nanoseconds by
the correct factor (1, 1e3, 1e6, 1e9, ...) and append the matching unit string
while preserving the existing formatting behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
perf/bench/asio/callback/accept_churn_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/callback/fan_out_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/http_server_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/io_context_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/callback/timer_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/accept_churn_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/benchmarks.hppis excluded by!**/bench/**perf/bench/asio/coroutine/fan_out_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/http_server_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/io_context_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/socket_latency_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/asio/coroutine/timer_bench.cppis excluded by!**/bench/**perf/bench/common/benchmark.hppis excluded by!**/bench/**perf/bench/common/suite.hppis excluded by!**/bench/**perf/bench/corosio/accept_churn_bench.cppis excluded by!**/bench/**perf/bench/corosio/benchmarks.hppis excluded by!**/bench/**perf/bench/corosio/fan_out_bench.cppis excluded by!**/bench/**perf/bench/corosio/http_server_bench.cppis excluded by!**/bench/**perf/bench/corosio/io_context_bench.cppis excluded by!**/bench/**perf/bench/corosio/socket_latency_bench.cppis excluded by!**/bench/**perf/bench/corosio/socket_throughput_bench.cppis excluded by!**/bench/**perf/bench/corosio/timer_bench.cppis excluded by!**/bench/**perf/bench/main.cppis excluded by!**/bench/**
📒 Files selected for processing (5)
CMakeLists.txtcmake/CorosioBuild.cmakeperf/CMakeLists.txtperf/common/native_includes.hppperf/common/perf.hpp
|
GCOVR code coverage report https://177.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-26 13:04:16 UTC |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmake/CorosioBuild.cmake (1)
49-55:⚠️ Potential issue | 🟡 MinorAdd a timeout to the first git probe too.
Line 49 runs
git rev-parsewithoutTIMEOUT, so configure can still block unexpectedly on problematic environments.🔧 Proposed fix
if(NOT DEFINED CACHE{BOOST_COROSIO_CAPY_TAG}) execute_process( COMMAND git rev-parse --abbrev-ref HEAD WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} + TIMEOUT 5 OUTPUT_VARIABLE _corosio_branch OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET RESULT_VARIABLE _git_result)#!/bin/bash # Verify timeout coverage for git execute_process probes in CorosioBuild.cmake sed -n '45,70p' cmake/CorosioBuild.cmake rg -n 'execute_process\\(|COMMAND git|TIMEOUT' cmake/CorosioBuild.cmake🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/CorosioBuild.cmake` around lines 49 - 55, The execute_process invocation that runs "git rev-parse --abbrev-ref HEAD" (which populates _corosio_branch and sets _git_result) needs a TIMEOUT to avoid blocking; update that execute_process call to include a suitable TIMEOUT value (e.g., TIMEOUT 10) so the git probe fails fast on hung environments and keep existing OUTPUT_STRIP_TRAILING_WHITESPACE, ERROR_QUIET and RESULT_VARIABLE handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/CorosioBuild.cmake`:
- Around line 193-199: The install-generation branch currently checks
boost_capy_FOUND to decide whether to emit standalone install rules, but that
variable can be unset when a pre-existing CMake target (Boost::capy) is provided
by a sibling project; change the guard to test for the target instead. Update
the conditional that currently reads "elseif(boost_capy_FOUND)" so it uses the
CMake TARGET lookup (e.g., if(TARGET Boost::capy)) and keep the body that
includes GNUInstallDirs and creates install rules (the
boost_install/HEADER_DIRECTORY block should remain under the
BOOST_SUPERPROJECT_VERSION branch). This ensures install rules are generated
whenever the Boost::capy target exists even if boost_capy_FOUND is not set.
---
Duplicate comments:
In `@cmake/CorosioBuild.cmake`:
- Around line 49-55: The execute_process invocation that runs "git rev-parse
--abbrev-ref HEAD" (which populates _corosio_branch and sets _git_result) needs
a TIMEOUT to avoid blocking; update that execute_process call to include a
suitable TIMEOUT value (e.g., TIMEOUT 10) so the git probe fails fast on hung
environments and keep existing OUTPUT_STRIP_TRAILING_WHITESPACE, ERROR_QUIET and
RESULT_VARIABLE handling unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/unit/CMakeLists.txtis excluded by!**/test/**
📒 Files selected for processing (4)
CMakeLists.txtREADME.mdcmake/CorosioBuild.cmakeperf/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- perf/CMakeLists.txt
| if(BOOST_SUPERPROJECT_VERSION AND NOT CMAKE_VERSION VERSION_LESS 3.13) | ||
| boost_install( | ||
| TARGETS ${_corosio_install_targets} | ||
| VERSION ${BOOST_SUPERPROJECT_VERSION} | ||
| HEADER_DIRECTORY include) | ||
| elseif(boost_capy_FOUND) | ||
| include(GNUInstallDirs) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the relevant sections of the CMake file
sed -n '40,90p' cmake/CorosioBuild.cmake
sed -n '184,210p' cmake/CorosioBuild.cmake
rg -n 'find_package\(boost_capy|boost_capy_FOUND|TARGET Boost::capy' cmake/CorosioBuild.cmakeRepository: cppalliance/corosio
Length of output: 3448
Use TARGET Boost::capy instead of boost_capy_FOUND to gate standalone install rules.
Line 198 gates install generation on boost_capy_FOUND, but this variable is never set when Boost::capy is a pre-existing target from a sibling/subproject (the find_package at line 42 is skipped when the target already exists at line 41). The install rules would incorrectly skip even though the dependency is available.
🔧 Proposed fix
- elseif(boost_capy_FOUND)
+ elseif(TARGET Boost::capy)
include(GNUInstallDirs)
include(CMakePackageConfigHelpers)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake/CorosioBuild.cmake` around lines 193 - 199, The install-generation
branch currently checks boost_capy_FOUND to decide whether to emit standalone
install rules, but that variable can be unset when a pre-existing CMake target
(Boost::capy) is provided by a sibling project; change the guard to test for the
target instead. Update the conditional that currently reads
"elseif(boost_capy_FOUND)" so it uses the CMake TARGET lookup (e.g., if(TARGET
Boost::capy)) and keep the body that includes GNUInstallDirs and creates install
rules (the boost_install/HEADER_DIRECTORY block should remain under the
BOOST_SUPERPROJECT_VERSION branch). This ensures install rules are generated
whenever the Boost::capy target exists even if boost_capy_FOUND is not set.
There was a problem hiding this comment.
The boost_capy_FOUND guard is intentional. It
distinguishes "capy is installed on the system via
find_package" from "capy was built locally via sibling
add_subdirectory or FetchContent." In the latter
cases, boost_capy isn't in any export set, so
generating install rules for corosio would produce
broken exports — CMake errors with requires target
"boost_capy" that is not in any export set. The guard
ensures standalone install rules are only emitted when
capy is already independently installed and findable.
There was a problem hiding this comment.
@sgerbino Thank you for the clarification! You're absolutely right—the boost_capy_FOUND guard correctly distinguishes between capy being installed as a findable package versus being built locally. Using TARGET Boost::capy would indeed trigger install rules when capy is provided by sibling add_subdirectory or FetchContent, leading to broken exports since those targets aren't part of any export set. The guard ensures standalone install rules are only generated when capy is independently installed and can be found by downstream consumers. I appreciate the explanation of the CMake packaging constraint.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
15-17: Consider using a semantic version for the fallback.The fallback version
1is unusual for CMake project versions, which typically follow semantic versioning (e.g.,1.0.0). This may cause issues withwrite_basic_package_version_filecompatibility checks if consumers expect a proper version format.💡 Suggested improvement
else() - set(BOOST_COROSIO_VERSION 1) + set(BOOST_COROSIO_VERSION 1.0.0) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 15 - 17, The fallback version set by BOOST_COROSIO_VERSION is currently a lone integer "1" which can break tools expecting semantic versions; change the fallback to a proper semantic version string (e.g., "1.0.0") wherever BOOST_COROSIO_VERSION is set in the CMakeLists (the else() branch that assigns BOOST_COROSIO_VERSION) so downstream calls like write_basic_package_version_file receive a valid semantic version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 15-17: The fallback version set by BOOST_COROSIO_VERSION is
currently a lone integer "1" which can break tools expecting semantic versions;
change the fallback to a proper semantic version string (e.g., "1.0.0") wherever
BOOST_COROSIO_VERSION is set in the CMakeLists (the else() branch that assigns
BOOST_COROSIO_VERSION) so downstream calls like write_basic_package_version_file
receive a valid semantic version.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/unit/CMakeLists.txtis excluded by!**/test/**
📒 Files selected for processing (4)
CMakeLists.txtREADME.mdcmake/CorosioBuild.cmakeperf/CMakeLists.txt
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #177 +/- ##
===========================================
+ Coverage 71.74% 76.13% +4.39%
===========================================
Files 88 94 +6
Lines 9059 10661 +1602
Branches 2362 2362
===========================================
+ Hits 6499 8117 +1618
- Misses 1581 1790 +209
+ Partials 979 754 -225
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
sgerbino
force-pushed
the
pr/bench
branch
4 times, most recently
from
deb07dd to
429e6df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/CorosioBuild.cmake`:
- Around line 21-35: The unconditional add_subdirectory(../.. ...) call when
BOOST_COROSIO_IS_ROOT is true can run in non-Boost parent layouts and break
configuration; before setting BOOST_INCLUDE_LIBRARIES/BOOST_EXCLUDE_LIBRARIES,
guard the add_subdirectory invocation by checking for a valid Boost superproject
(e.g., test for the existence of ../../CMakeLists.txt and
../../libs/corosio/CMakeLists.txt) and only set CMAKE_FOLDER and call
add_subdirectory(../.. ${CMAKE_CURRENT_BINARY_DIR}/deps/boost EXCLUDE_FROM_ALL)
if those files exist; otherwise skip the add_subdirectory path so the
FetchContent fallback can run. Ensure checks are placed in the same conditional
block that references BOOST_COROSIO_IS_ROOT and use the same variables
(set(BOOST_INCLUDE_LIBRARIES ...), set(BOOST_EXCLUDE_LIBRARIES corosio),
unset(CMAKE_FOLDER)) so control flow remains correct.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/Jamfileis excluded by!**/build/**test/unit/CMakeLists.txtis excluded by!**/test/**
📒 Files selected for processing (5)
.github/workflows/ci.ymlCMakeLists.txtREADME.mdcmake/CorosioBuild.cmakeperf/CMakeLists.txt
Move install rules, TLS provider discovery, and MrDocs setup out of CMakeLists.txt into reusable functions in cmake/CorosioBuild.cmake: - corosio_install(): install rules for superproject and standalone builds - corosio_find_tls_provider(): parameterized find/link/platform-deps for TLS backends (replaces duplicated WolfSSL/OpenSSL blocks) - corosio_setup_mrdocs(): MrDocs synthetic translation unit target Also condense repeated target_include_directories and target_compile_definitions calls using generator expressions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/CorosioBuild.cmake (1)
43-49: Consider adding timeout to local git command for consistency.The second
execute_process(line 51-58) hasTIMEOUT 30, but this firstexecute_processforgit rev-parsedoes not. Whilerev-parseoperates on the local repository and is typically fast, adding a short timeout would provide consistent protection against edge cases (e.g., network-mounted filesystems, corrupted git state).🔧 Suggested improvement
execute_process( COMMAND git rev-parse --abbrev-ref HEAD WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} + TIMEOUT 5 OUTPUT_VARIABLE _corosio_branch OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET RESULT_VARIABLE _git_result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/CorosioBuild.cmake` around lines 43 - 49, Add a short TIMEOUT to the execute_process call that runs git rev-parse so it cannot hang (mirror the pattern used in the later execute_process with TIMEOUT 30); update the execute_process invocation that sets OUTPUT_VARIABLE _corosio_branch (the git rev-parse --abbrev-ref HEAD call) to include a TIMEOUT value (e.g., 30) and ensure RESULT_VARIABLE _git_result remains present so timeouts are reported the same way as the other git call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/CorosioBuild.cmake`:
- Around line 67-68: The CACHE set for BOOST_COROSIO_CAPY_TAG is executed
unconditionally using _default_capy_tag which may be undefined and can overwrite
a user-provided cache value; move the line set(BOOST_COROSIO_CAPY_TAG
"${_default_capy_tag}" CACHE STRING ...) into the same conditional that defines
_default_capy_tag (i.e., only call set(...) when the block that establishes
_default_capy_tag runs) or guard it with an if(NOT DEFINED
BOOST_COROSIO_CAPY_TAG) check so you never write the cache with an
empty/undefined _default_capy_tag; reference BOOST_COROSIO_CAPY_TAG,
_default_capy_tag and the CACHE assignment when making the change.
---
Nitpick comments:
In `@cmake/CorosioBuild.cmake`:
- Around line 43-49: Add a short TIMEOUT to the execute_process call that runs
git rev-parse so it cannot hang (mirror the pattern used in the later
execute_process with TIMEOUT 30); update the execute_process invocation that
sets OUTPUT_VARIABLE _corosio_branch (the git rev-parse --abbrev-ref HEAD call)
to include a TIMEOUT value (e.g., 30) and ensure RESULT_VARIABLE _git_result
remains present so timeouts are reported the same way as the other git call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/unit/CMakeLists.txtis excluded by!**/test/**
📒 Files selected for processing (5)
.github/workflows/ci.ymlCMakeLists.txtREADME.mdcmake/CorosioBuild.cmakeperf/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- perf/CMakeLists.txt
| set(BOOST_COROSIO_CAPY_TAG "${_default_capy_tag}" CACHE STRING | ||
| "Git tag/branch for capy when fetching via FetchContent") |
There was a problem hiding this comment.
Cache variable set should be inside the conditional block.
The set(BOOST_COROSIO_CAPY_TAG ...) at lines 67-68 executes unconditionally, even when CACHE{BOOST_COROSIO_CAPY_TAG} is already defined (line 42). When the if block is skipped, _default_capy_tag is undefined, which would overwrite a user-provided cache value with an empty string.
🐛 Proposed fix
if(NOT DEFINED _default_capy_tag)
set(_default_capy_tag "develop")
endif()
+ set(BOOST_COROSIO_CAPY_TAG "${_default_capy_tag}" CACHE STRING
+ "Git tag/branch for capy when fetching via FetchContent")
endif()
- set(BOOST_COROSIO_CAPY_TAG "${_default_capy_tag}" CACHE STRING
- "Git tag/branch for capy when fetching via FetchContent")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmake/CorosioBuild.cmake` around lines 67 - 68, The CACHE set for
BOOST_COROSIO_CAPY_TAG is executed unconditionally using _default_capy_tag which
may be undefined and can overwrite a user-provided cache value; move the line
set(BOOST_COROSIO_CAPY_TAG "${_default_capy_tag}" CACHE STRING ...) into the
same conditional that defines _default_capy_tag (i.e., only call set(...) when
the block that establishes _default_capy_tag runs) or guard it with an if(NOT
DEFINED BOOST_COROSIO_CAPY_TAG) check so you never write the cache with an
empty/undefined _default_capy_tag; reference BOOST_COROSIO_CAPY_TAG,
_default_capy_tag and the CACHE assignment when making the change.
There was a problem hiding this comment.
- If BOOST_COROSIO_CAPY_TAG is already in the cache
(user-provided via -D), CMake's set(... CACHE ...)
without FORCE is a no-op — it never overwrites an
existing cache entry. - If it's not in the cache, the if(NOT DEFINED
CACHE{BOOST_COROSIO_CAPY_TAG}) block on line 42 always
runs first and guarantees _default_capy_tag is
defined (line 63-64 falls back to "develop").
There was a problem hiding this comment.
@sgerbino You're absolutely right on both counts—thank you for the clarification!
-
CMake cache semantics:
set(... CACHE ...)withoutFORCEis indeed a no-op when the cache entry already exists, so it never overwrites a user-provided value. -
Guaranteed fallback: The conditional block ensures
_default_capy_tagis always defined (with the"develop"fallback at lines 63–64) before the cache assignment runs.
My concern was unfounded. The code is safe as-is. Apologies for the noise! 🙏
🧠 Learnings used
Learnt from: CR
Repo: cppalliance/corosio PR: 0
File: .cursor/rules/portable-headers.mdc:0-0
Timestamp: 2026-01-22T19:54:36.343Z
Learning: Applies to include/boost/corosio/**/*.{h,hpp} : Platform-specific macros and types that require platform-specific headers are prohibited in public headers
Learnt from: CR
Repo: cppalliance/corosio PR: 0
File: .cursor/rules/portable-headers.mdc:0-0
Timestamp: 2026-01-22T19:54:36.343Z
Learning: Applies to include/boost/corosio/**/*.{h,hpp} : Public headers in `include/boost/corosio/` and all subdirectories MUST NOT include platform-specific headers (Windows headers like `<windows.h>`, `<WinSock2.h>`, `<Ws2tcpip.h>`, `<MSWSock.h>` or Unix/POSIX headers like `<sys/socket.h>`, `<netinet/in.h>`, `<arpa/inet.h>`, `<unistd.h>`, `<sys/types.h>`)
2 participants
Resolves #178.
Summary by CodeRabbit
New Features
Chores