Architecture cleanup: header relocation, naming, docs, and formatting#182
Merged
sgerbino merged 1 commit intocppalliance:developfrom Feb 27, 2026
Merged
Conversation
Enforce layer boundaries by relocating platform-dependent headers out of the type-erased detail/ scope. Standardize naming across the native layer, fix stale documentation, and apply project-wide formatting. Header relocation: - Move endpoint_convert.hpp and make_err.hpp from detail/ to native/detail/ (both include platform headers, consumed only by native backends) - Rename io_buffer_param.hpp to buffer_param.hpp and move from io/ to detail/ (concrete value type, not an abstract base) - Remove buffer_param from the umbrella header (not user-facing) Naming and API consistency: - Rename impl_type/get_impl() to implementation_type/get() in all native headers to match the convention used at the abstract and concrete layers - Rename io_buffer_param.cpp test to buffer_param.cpp to match the renamed header - Pass std::stop_token and tls_context by const reference where only read (removes 8 unnecessary-value-param suppressions) Code quality: - Remove unused includes detected via clangd/clang-tidy - Fix clang-tidy diagnostics: suppress bugprone-unused-return-value on tls_context setup calls, fix widening conversions, add reserve() before emplace_back loops, add std::move for pass-by-value tls_context - Convert inline NOLINT to NOLINTNEXTLINE where the trailing comment caused clang-format to split lines - Fix stale "Defined in sockets.cpp" comments in op headers to reference actual service headers after header/source merges - Merge duplicate block comments in kqueue_socket_service.hpp and posix_signal_service.hpp - Move includes from posix_resolver.hpp to posix_resolver_service.hpp where they are used Design documentation: - Add "Implementation leaf nodes" subsection to the native layer section covering inheritance chains, leaf node table, common structural patterns, IOCP two-class split, and timer special case - Fix native layer code example to match actual implementation (template<auto Backend>, implementation_type/get() naming, backend_type trait aliases) - Add native_resolver to types table, fix backend tag and implementation header paths, update platform source layout Formatting: - Tune .clang-format: BinPackArguments false, AllowAllArgumentsOnNextLine false, AllowAllParametersOfDeclarationOnNextLine false, AlignConsecutiveAssignments false - Apply clang-format project-wide
📝 WalkthroughWalkthroughThis PR systematically renames the public type Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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://182.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-27 19:36:06 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/corosio/socket_option.hpp (1)
394-400:⚠️ Potential issue | 🟡 MinorKeep
resizeparameter docs and signature aligned.Line 400 declares
void resize(std::size_t)but the doc block still documents@param s. Please either name the parameter or remove the@paramentry to avoid stale API docs.Suggested fix
- void resize(std::size_t) noexcept {} + void resize(std::size_t s) noexcept + { + (void)s; + }As per coding guidelines: "
@param documentation for each parameter" in public-header documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/socket_option.hpp` around lines 394 - 400, The docblock for the public function resize documents a parameter named "s" but the declaration is void resize(std::size_t) with an unnamed parameter; update the declaration to name the parameter (e.g., void resize(std::size_t s) noexcept {}) so the `@param` s doc is accurate (or alternatively remove the `@param` entry if you prefer no parameter name); locate the resize function in include/boost/corosio/socket_option.hpp and make the signature and docs consistent.
🧹 Nitpick comments (2)
include/boost/corosio/wolfssl_stream.hpp (1)
81-84: Clarifyctxlifetime/copy semantics in constructor docs.Now that
ctxis aconst&, please explicitly state whether construction copies the TLS configuration immediately or stores/reuses any part ofctx, so caller lifetime expectations are unambiguous.As per coding guidelines "@param documentation for each parameter — Including ownership/lifetime semantics".
Also applies to: 99-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/wolfssl_stream.hpp` around lines 81 - 84, Update the constructor documentation for wolfssl_stream (and the overloaded constructor at the other occurrence) to explicitly state the lifetime and ownership semantics of the parameter ctx: indicate whether the constructor copies the TLS configuration from the provided const& ctx into internal storage (and thus the caller may destroy the original after construction) or whether it merely stores a reference/pointer to ctx (requiring the caller to keep ctx alive for the wolfssl_stream lifetime); reference the constructor signature (wolfssl_stream::wolfssl_stream(..., const TLSContext& ctx) or similar) so readers can find the exact function and ensure the `@param` ctx lines (both locations) mention whether a deep/shallow copy is made and who owns/retains the configuration.include/boost/corosio/native/detail/epoll/epoll_op.hpp (1)
225-234: Standardizestd::stop_tokenparameter passing convention across backends.Inconsistency exists in how
start()methods accept the stop token. POSIX backends (epoll_op.hpp, select_op.hpp, posix_resolver.hpp) usestd::stop_token const&, while non-POSIX backends (win_overlapped_op.hpp, kqueue_op.hpp) pass by value. Consider standardizing to const reference across all backends for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/boost/corosio/native/detail/epoll/epoll_op.hpp` around lines 225 - 234, The start(std::stop_token const& token, epoll_socket* impl) signature should be used consistently across backends: change the analogous start(...) signatures in non-POSIX backends (e.g., win_overlapped_op::start and kqueue_op::start) from taking std::stop_token by value to taking std::stop_token const&; update any corresponding forward declarations and call sites to accept and forward a const& (no semantic changes otherwise), and ensure stop_cb emplacement logic (canceller{this}) remains compatible with the const& parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@include/boost/corosio/socket_option.hpp`:
- Around line 394-400: The docblock for the public function resize documents a
parameter named "s" but the declaration is void resize(std::size_t) with an
unnamed parameter; update the declaration to name the parameter (e.g., void
resize(std::size_t s) noexcept {}) so the `@param` s doc is accurate (or
alternatively remove the `@param` entry if you prefer no parameter name); locate
the resize function in include/boost/corosio/socket_option.hpp and make the
signature and docs consistent.
---
Nitpick comments:
In `@include/boost/corosio/native/detail/epoll/epoll_op.hpp`:
- Around line 225-234: The start(std::stop_token const& token, epoll_socket*
impl) signature should be used consistently across backends: change the
analogous start(...) signatures in non-POSIX backends (e.g.,
win_overlapped_op::start and kqueue_op::start) from taking std::stop_token by
value to taking std::stop_token const&; update any corresponding forward
declarations and call sites to accept and forward a const& (no semantic changes
otherwise), and ensure stop_cb emplacement logic (canceller{this}) remains
compatible with the const& parameter.
In `@include/boost/corosio/wolfssl_stream.hpp`:
- Around line 81-84: Update the constructor documentation for wolfssl_stream
(and the overloaded constructor at the other occurrence) to explicitly state the
lifetime and ownership semantics of the parameter ctx: indicate whether the
constructor copies the TLS configuration from the provided const& ctx into
internal storage (and thus the caller may destroy the original after
construction) or whether it merely stores a reference/pointer to ctx (requiring
the caller to keep ctx alive for the wolfssl_stream lifetime); reference the
constructor signature (wolfssl_stream::wolfssl_stream(..., const TLSContext&
ctx) or similar) so readers can find the exact function and ensure the `@param`
ctx lines (both locations) mention whether a deep/shallow copy is made and who
owns/retains the configuration.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (23)
doc/design/physical-structure.mdis excluded by!**/doc/**doc/modules/ROOT/pages/4.guide/4n.buffers.adocis excluded by!**/doc/**include/boost/corosio/test/mocket.hppis excluded by!**/test/**test/unit/acceptor.cppis excluded by!**/test/**test/unit/cancel.cppis excluded by!**/test/**test/unit/endpoint.cppis excluded by!**/test/**test/unit/io_buffer_param.cppis excluded by!**/test/**test/unit/io_context.cppis excluded by!**/test/**test/unit/ipv4_address.cppis excluded by!**/test/**test/unit/ipv6_address.cppis excluded by!**/test/**test/unit/native/iocp/iocp_shutdown.cppis excluded by!**/test/**test/unit/native/native_cancel.cppis excluded by!**/test/**test/unit/openssl_stream.cppis excluded by!**/test/**test/unit/resolver.cppis excluded by!**/test/**test/unit/socket.cppis excluded by!**/test/**test/unit/socket_stress.cppis excluded by!**/test/**test/unit/stream_tests.hppis excluded by!**/test/**test/unit/tcp_server.cppis excluded by!**/test/**test/unit/test_utils.hppis excluded by!**/test/**test/unit/timer.cppis excluded by!**/test/**test/unit/tls_stream_stress.cppis excluded by!**/test/**test/unit/tls_stream_tests.hppis excluded by!**/test/**test/unit/wolfssl_stream.cppis excluded by!**/test/**
📒 Files selected for processing (58)
.cursor/rules/portable-headers.mdcinclude/boost/corosio.hppinclude/boost/corosio/cancel.hppinclude/boost/corosio/detail/acceptor_service.hppinclude/boost/corosio/detail/buffer_param.hppinclude/boost/corosio/detail/cancel_at_awaitable.hppinclude/boost/corosio/detail/platform.hppinclude/boost/corosio/detail/socket_service.hppinclude/boost/corosio/detail/thread_local_ptr.hppinclude/boost/corosio/detail/timeout_coro.hppinclude/boost/corosio/detail/timer_service.hppinclude/boost/corosio/io/io_read_stream.hppinclude/boost/corosio/io/io_stream.hppinclude/boost/corosio/io/io_write_stream.hppinclude/boost/corosio/ipv6_address.hppinclude/boost/corosio/native/detail/endpoint_convert.hppinclude/boost/corosio/native/detail/epoll/epoll_acceptor.hppinclude/boost/corosio/native/detail/epoll/epoll_acceptor_service.hppinclude/boost/corosio/native/detail/epoll/epoll_op.hppinclude/boost/corosio/native/detail/epoll/epoll_scheduler.hppinclude/boost/corosio/native/detail/epoll/epoll_socket.hppinclude/boost/corosio/native/detail/epoll/epoll_socket_service.hppinclude/boost/corosio/native/detail/iocp/win_acceptor.hppinclude/boost/corosio/native/detail/iocp/win_acceptor_service.hppinclude/boost/corosio/native/detail/iocp/win_overlapped_op.hppinclude/boost/corosio/native/detail/iocp/win_resolver.hppinclude/boost/corosio/native/detail/iocp/win_scheduler.hppinclude/boost/corosio/native/detail/iocp/win_socket.hppinclude/boost/corosio/native/detail/iocp/win_sockets.hppinclude/boost/corosio/native/detail/iocp/win_wsa_init.hppinclude/boost/corosio/native/detail/kqueue/kqueue_acceptor.hppinclude/boost/corosio/native/detail/kqueue/kqueue_acceptor_service.hppinclude/boost/corosio/native/detail/kqueue/kqueue_scheduler.hppinclude/boost/corosio/native/detail/kqueue/kqueue_socket.hppinclude/boost/corosio/native/detail/kqueue/kqueue_socket_service.hppinclude/boost/corosio/native/detail/make_err.hppinclude/boost/corosio/native/detail/posix/posix_resolver.hppinclude/boost/corosio/native/detail/posix/posix_resolver_service.hppinclude/boost/corosio/native/detail/select/select_acceptor.hppinclude/boost/corosio/native/detail/select/select_acceptor_service.hppinclude/boost/corosio/native/detail/select/select_op.hppinclude/boost/corosio/native/detail/select/select_scheduler.hppinclude/boost/corosio/native/detail/select/select_socket.hppinclude/boost/corosio/native/detail/select/select_socket_service.hppinclude/boost/corosio/native/native_cancel.hppinclude/boost/corosio/native/native_socket_option.hppinclude/boost/corosio/native/native_tcp.hppinclude/boost/corosio/openssl_stream.hppinclude/boost/corosio/socket_option.hppinclude/boost/corosio/tcp.hppinclude/boost/corosio/tcp_acceptor.hppinclude/boost/corosio/tcp_socket.hppinclude/boost/corosio/wolfssl_stream.hppsrc/corosio/src/endpoint.cppsrc/corosio/src/socket_option.cppsrc/corosio/src/tcp.cppsrc/corosio/src/tcp_acceptor.cppsrc/corosio/src/tcp_socket.cpp
💤 Files with no reviewable changes (3)
- include/boost/corosio/ipv6_address.hpp
- src/corosio/src/endpoint.cpp
- include/boost/corosio.hpp
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #182 +/- ##
===========================================
+ Coverage 76.22% 76.38% +0.16%
===========================================
Files 98 101 +3
Lines 10467 10572 +105
Branches 2383 2385 +2
===========================================
+ Hits 7978 8075 +97
- Misses 1762 1774 +12
+ Partials 727 723 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
GCOVR code coverage report https://182.corosio.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-27 19:42:51 UTC |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Enforce layer boundaries by relocating platform-dependent headers out of the type-erased detail/ scope. Standardize naming across the native layer, fix stale documentation, and apply project-wide formatting.
Header relocation:
Naming and API consistency:
Code quality:
Design documentation:
Formatting:
Summary by CodeRabbit
Release Notes
Breaking Changes
io_buffer_paramtobuffer_paramacross I/O stream APIs.Enhancements
Documentation