Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces mock-only example streams with paired test streams; adds eager coroutine writer boost::capy::write_now; introduces partial-I/O awaitables (read_some / write_some) and atomic write_eof(buffers); migrates many coroutine return types to io_task<...>; moves prepared-buffer handling to buffer_array; replaces ec.failed() with if (ec). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant write_now as "write_now<Stream>"
participant Stream
participant Scheduler
Caller->>write_now: operator()(buffers)
Caller->>Scheduler: co_await op_type
Scheduler->>write_now: await_suspend(op)
write_now->>Stream: attempt repeated write_some (synchronous fast-path)
alt synchronous completion
Stream-->>write_now: bytes_written/result
write_now-->>Scheduler: await_resume(result)
else needs async slow path
write_now->>Stream: construct underlying write awaitable (slow path)
Stream-->>Scheduler: suspend/resume underlying awaitable
Stream-->>write_now: bytes_written/result
write_now-->>Scheduler: await_resume(result)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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://133.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-07 16:38:18 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: 1
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/buffered_write_stream.hpp`:
- Line 74: Replace the non-existent .failed() call with the boolean truthiness
idiom used elsewhere: change the check on the std::error_code variable ec in
buffered_write_stream.hpp (inside the BufferedWriteStream
write/flush/error-handling code where ec is inspected) from if( ec.failed() ) to
if (ec) so it compiles and matches the PR convention.
🧹 Nitpick comments (6)
include/boost/capy/io/write_now.hpp (1)
184-204: Custom allocator looks correct but consider documenting the invariant.The
operator new/ no-opoperator deletepair correctly implements the frame cache. If::operator new(size)throws,self.cached_frame_still points at the old (smaller but valid) block—no leak. One subtle invariant: afterh_.destroy()in~op_type, the memory atcached_frame_is deallocated from the coroutine's perspective but retained for reuse. This is non-obvious and could benefit from a brief comment inoperator delete.include/boost/capy/io/buffered_write_stream.hpp (3)
618-660: Precondition violation onbuf.size() > capacity()causes an infinite loop.The javadoc at line 609 documents
buf.size() <= capacity()as a precondition, but there's no runtime check. If violated, the drain loops (lines 629 and 649) can never free enough space and will loop until the stream errors or cancellation occurs. Consider adding an assertion or throwing on violation.Suggested defensive check
op_type write(const_buffer buf) { + BOOST_ASSERT(buf.size() <= capacity()); // Fast path: room in circular buffer if(cb_.capacity() >= buf.size())
453-468: Frame-cachingoperator new/ no-opoperator deletemirrorswrite_now— consider extracting shared logic.This allocation pattern (lines 453–473) is nearly identical to
write_now::op_type::promise_type(write_now.hpp lines 184–204). If the pattern is expected to appear in more places, a reusable CRTP base or helper could reduce duplication.
154-169:unhandled_exceptionselectively catchessystem_errorfor shortcircuit — intentional but worth a brief comment.The design relies on
op_type::await_resumethrowingsystem_error(line 534) to trigger the catch on line 160. Other exceptions propagate normally. This coupling betweenop_type::await_resumeandsession_task::unhandled_exceptionis the core of the shortcircuit mechanism and could benefit from a single-line comment linking the two.example/custom-dynamic-buffer/custom_dynamic_buffer.cpp (1)
13-13: Unused include:<boost/capy/test/fuse.hpp>is not referenced in this file.
test::fuseis not used anywhere in this example. Consider removing the stale include.Suggested fix
`#include` <boost/capy/test/run_blocking.hpp> `#include` <boost/capy/test/stream.hpp> -#include <boost/capy/test/fuse.hpp> `#include` <boost/capy/cond.hpp>example/type-erased-echo/main.cpp (1)
13-13: Unused include:<boost/capy/test/fuse.hpp>is not referenced in this file.No
test::fuseusage here. Consider removing to keep the example minimal.Suggested fix
`#include` <boost/capy/test/stream.hpp> -#include <boost/capy/test/fuse.hpp> `#include` <boost/capy/test/run_blocking.hpp>
| { | ||
| buffered_write_stream bw( stream, 4096 ); | ||
| auto ec = co_await do_writes( bw ); | ||
| if( ec.failed() ) |
There was a problem hiding this comment.
Example uses .failed() but the PR switches to if(ec) everywhere else.
std::error_code doesn't have a .failed() member. If this relies on a project-specific extension, it still conflicts with the PR's direction of using boolean truthiness. The example should match the new idiom.
Suggested fix
- if( ec.failed() )
+ if( ec )🤖 Prompt for AI Agents
In `@include/boost/capy/io/buffered_write_stream.hpp` at line 74, Replace the
non-existent .failed() call with the boolean truthiness idiom used elsewhere:
change the check on the std::error_code variable ec in buffered_write_stream.hpp
(inside the BufferedWriteStream write/flush/error-handling code where ec is
inspected) from if( ec.failed() ) to if (ec) so it compiles and matches the PR
convention.
vinniefalco
force-pushed
the
develop
branch
2 times, most recently
from
f8e7050 to
1b8280a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
example/stream-pipeline/stream_pipeline.cpp (1)
20-22:⚠️ Potential issue | 🟡 MinorStale comment:
pull()methods now returnio_task<>, nottask<>.The file-level overview still says "task<> coroutines" but the pull methods were updated to return
io_task<std::span<const_buffer>>.Proposed fix
-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowing // them to properly co_await the upstream source.include/boost/capy/io/any_buffer_sink.hpp (1)
684-698:⚠️ Potential issue | 🟡 MinorUnbounded retry when
preparealways returns empty after flush.If the underlying sink's
prepareperpetually yields an empty span even after a successfulcommit(0), bothwriteandwrite_eofloop forever (asynchronously). Consider adding a retry ceiling or converting a no-progress iteration into an error to avoid an infinite async loop.include/boost/capy/io/any_write_sink.hpp (2)
396-406:⚠️ Potential issue | 🟡 MinorDestructor skips active awaitable cleanup.
If the object is destroyed while a write operation is in-flight (e.g., the owning coroutine is destroyed without resuming), the awaitable living in
cached_awaitable_is freed via raw::operator deletewithout calling the awaitable's destructor throughactive_write_ops_->destroy/active_eof_ops_->destroy. Consider documenting that no operations may be in-flight at destruction time, or adding a defensive check:if(active_write_ops_) active_write_ops_->destroy(cached_awaitable_); if(active_eof_ops_) active_eof_ops_->destroy(cached_awaitable_);
448-452:⚠️ Potential issue | 🟡 MinorOver-aligned types:
awaitable_alignis computed but never used during allocation.
vtablestoresawaitable_align(line 299) andvtable_for_implcomputesmax_awaitable_align(lines 380-383), but both allocations use plain::operator new(size), which only guarantees__STDCPP_DEFAULT_NEW_ALIGNMENT__. IfSor the awaitable type is over-aligned, the placement-new on lines 449 and 329 is UB. The aligned form::operator new(size, std::align_val_t{align})would be needed (with matching::operator delete).This is unlikely to bite with typical stream types but is a latent correctness gap.
include/boost/capy/io/push_to.hpp (1)
141-146:⚠️ Potential issue | 🟠 MajorBug: partial write bytes lost from
totalon error.When
write_somereturns an error withn > 0(bytes written before the failure), those bytes are not added tototalbefore returning. This is inconsistent with the WriteSink overload (lines 79-83) which addsntototalbefore checking the error, and with the documented return semantics ("n is the total number of bytes transferred before the error").Proposed fix
auto [write_ec, n] = co_await stream.write_some(bufs); + total += n; + source.consume(n); if(write_ec) - co_return {write_ec, total}; - - total += n; - source.consume(n); + co_return {write_ec, total}; }
🧹 Nitpick comments (6)
include/boost/capy/io/any_buffer_source.hpp (1)
225-242: Javadoc forread_someis consistent with the existingreaddoc, but both are missing several guideline-required sections.Per the coding guidelines for async/awaitable function javadoc, the following are absent from this new declaration (and similarly absent from
read):
- Completion conditions (
@lilist: e.g., "One or more bytes are copied", "An error occurs", "EOF is reached")- Error conditions (notable
error_codevalues, e.g.,error::eof,capy::cond::canceled)- Cancellation support (whether stop_token /
cancel()is supported)Consider adding at least the error/completion conditions since
read_someis a new public API and callers need to know whenerror::eofvs. an empty read is returned. As per coding guidelines: "Document completion conditions in async/awaitable function javadoc using bulleted@lilist" and "Document error conditions in async/awaitable function javadoc, listing notable error_code values and conditions."include/boost/capy/concept/dynamic_buffer.hpp (1)
40-40: Nitpick:size_tis unqualified here vsstd::size_tin the@codeblocks at lines 85 and 133.Minor inconsistency in the instruction comment. The
@codeblocks correctly usestd::size_t.include/boost/capy/io/any_buffer_sink.hpp (1)
711-747:write_eofloop body is identical towrite— consider extracting the shared loop.The for-loop in
write_eof(lines 718–740) is a character-for-character duplicate of the one inwrite(lines 684–706). The only difference is thecommit_eof()epilogue. Extracting the drain loop into a private helper would eliminate this duplication and make future fixes apply to both paths.Sketch
+// Private helper: drain all of bp into the sink via prepare/commit. +// Returns {ec, total_bytes_committed}. +template<ConstBufferSequence CB> +io_task<std::size_t> +any_buffer_sink::write_all_(buffer_param<CB>& bp) +{ + std::size_t total = 0; + for(;;) + { + auto src = bp.data(); + if(src.empty()) + break; + mutable_buffer arr[detail::max_iovec_]; + auto dst_bufs = prepare(arr); + if(dst_bufs.empty()) + { + auto [ec] = co_await commit(0); + if(ec) + co_return {ec, total}; + continue; + } + auto n = buffer_copy(dst_bufs, src); + auto [ec] = co_await commit(n); + if(ec) + co_return {ec, total}; + bp.consume(n); + total += n; + } + co_return {{}, total}; +}Then
writeandwrite_eofboth delegate towrite_all_, withwrite_eofappendingcommit_eof().include/boost/capy/io/any_write_sink.hpp (1)
599-624: Consider extracting the shared drain loop.The buffer-draining loop (lines 606-617) is identical to the one in
write(CB)(lines 583-594). If a private helper coroutine (e.g.,write_all_) were extracted to drain all buffers and return{ec, total}, bothwriteandwrite_eof(CB)could reuse it, reducing duplication.include/boost/capy/io/push_to.hpp (1)
39-39:@returnformat deviates from the coding guideline.The guideline prescribes:
An awaitable yielding \(error_code,std::size_t)`. This file uses "A task that yields(std::error_code, std::size_t)" — both the phrasing ("A task that yields" → "An awaitable yielding") and the type name (std::error_code→error_code) differ. Same applies to line 104. As per coding guidelines,io_result<T1,T2,T3>return types should use the format: 'An awaitable yielding(error_code, T1, T2, T3)`' — include only type names without internal spaces.include/boost/capy/io/pull_from.hpp (1)
41-41: Same@returnformat deviation aspush_to.hpp.Lines 41 and 122 use "A task that yields
(std::error_code, std::size_t)" instead of the prescribed "An awaitable yielding(error_code,std::size_t)". As per coding guidelines,io_result<T1,T2,T3>return types should use the format: 'An awaitable yielding(error_code, T1, T2, T3)' — include only type names without internal spaces.
| template<ConstBufferSequence CB> | ||
| task<io_result<std::size_t>> | ||
| any_buffer_sink::write(CB buffers) | ||
| io_task<std::size_t> | ||
| any_buffer_sink::write_some(CB buffers) | ||
| { | ||
| return write(buffers, false); | ||
| buffer_param<CB> bp(buffers); | ||
| auto src = bp.data(); | ||
| if(src.empty()) | ||
| co_return {{}, 0}; | ||
|
|
||
| mutable_buffer arr[detail::max_iovec_]; | ||
| auto dst_bufs = prepare(arr); | ||
| if(dst_bufs.empty()) | ||
| { | ||
| auto [ec] = co_await commit(0); | ||
| if(ec) | ||
| co_return {ec, 0}; | ||
| // Retry after flush | ||
| dst_bufs = prepare(arr); | ||
| if(dst_bufs.empty()) | ||
| co_return {{}, 0}; | ||
| } | ||
|
|
||
| auto n = buffer_copy(dst_bufs, src); | ||
| auto [ec] = co_await commit(n); | ||
| if(ec) | ||
| co_return {ec, 0}; | ||
| co_return {{}, n}; | ||
| } |
There was a problem hiding this comment.
write_some can return success with 0 bytes written, which may violate caller expectations.
Lines 666–667: if prepare returns empty after a successful flush, this returns {{}, 0}. Typical write_some semantics guarantee either ≥ 1 byte written or an error. A caller that loops write_some until all data is consumed will spin infinitely if the sink transiently has no space.
Consider returning an error (e.g., a "would block" or "no buffer space" condition) instead of success with 0, or documenting that callers must handle the 0-byte success case.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/boost/capy/io/any_buffer_sink.hpp (1)
690-698:⚠️ Potential issue | 🟠 MajorUnbounded retry when
prepareperpetually returns empty buffers.In both
write(line 692→697) andwrite_eof(line 726→731), ifpreparereturns empty,commit(0)is called to flush and then the loop retries. If the underlying sink never produces space after flushing (e.g., a broken or zero-capacity sink), this loops forever without yielding an error.Consider adding a retry limit or returning an appropriate error condition after a bounded number of empty-prepare cycles.
Also applies to: 724-732
include/boost/capy/io/any_write_sink.hpp (1)
396-406:⚠️ Potential issue | 🔴 CriticalDestructor must destroy in-flight awaitable before freeing
cached_awaitable_.The destructor leaves in-flight awaitables undestroyed. If the wrapper is destroyed while an async operation is suspended,
active_write_ops_oractive_eof_ops_may be non-null and the awaitable will be freed without calling its destructor, leaking resources. This pattern is already correctly implemented inany_write_streamandany_read_stream. Sinceany_write_sinkuses two separate active operation pointers, the fix should be:if(cached_awaitable_) { if(active_write_ops_) active_write_ops_->destroy(cached_awaitable_); else if(active_eof_ops_) active_eof_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); }
🧹 Nitpick comments (7)
example/stream-pipeline/stream_pipeline.cpp (3)
74-74: Return type migration toio_tasklooks correct.The
co_returnexpressions throughout the body produce{error_code, span}aggregates consistent withio_task<std::span<const_buffer>>.However, the comment on line 73 still says
"returns task<>"— it should be updated to reflectio_task<>.Suggested comment update
- // BufferSource::pull - returns task<> to enable co_await on upstream + // BufferSource::pull - returns io_task<> to enable co_await on upstream
173-173: Same clean migration; same stale comment on line 172.Suggested comment update
- // BufferSource::pull - returns task<> to enable co_await on upstream + // BufferSource::pull - returns io_task<> to enable co_await on upstream
20-22: File-level comment referencestask<>coroutines; now stale after theio_taskmigration.Lines 20–22 describe the transforms as using
task<>coroutines. Consider updating to match the new return type.Suggested update
-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/any_buffer_sink.hpp (1)
684-709:writeandwrite_eofshare a duplicated prepare/copy/commit loop.The bodies of
write(lines 684–706) andwrite_eof(lines 718–740) are identical except thatwrite_eofappendscommit_eof(). Consider composingwrite_eofon top ofwrite:♻️ Suggested refactor
template<ConstBufferSequence CB> io_task<std::size_t> any_buffer_sink::write_eof(CB buffers) { - buffer_param<CB> bp(buffers); - std::size_t total = 0; - - for(;;) - { - auto src = bp.data(); - if(src.empty()) - break; - - mutable_buffer arr[detail::max_iovec_]; - auto dst_bufs = prepare(arr); - if(dst_bufs.empty()) - { - auto [ec] = co_await commit(0); - if(ec) - co_return {ec, total}; - continue; - } - - auto n = buffer_copy(dst_bufs, src); - auto [ec] = co_await commit(n); - if(ec) - co_return {ec, total}; - bp.consume(n); - total += n; - } - - auto [ec] = co_await commit_eof(); - if(ec) - co_return {ec, total}; - - co_return {{}, total}; + auto [ec, total] = co_await write(buffers); + if(ec) + co_return {ec, total}; + auto [eof_ec] = co_await commit_eof(); + if(eof_ec) + co_return {eof_ec, total}; + co_return {{}, total}; }Also applies to: 718-747
include/boost/capy/read.hpp (1)
133-156: Potentialstd::size_toverflow in 1.5× growth calculation.Line 154 and 220:
amount = amount / 2 + amountcan overflowstd::size_twhenamountexceedsSIZE_MAX * 2 / 3. In practiceprepare()is likely to bound this, but a cap would make the intent explicit:amount = amount / 2 + amount; // could wrap aroundPossible defensive cap
- if(n == mb_size) - amount = amount / 2 + amount; + if(n == mb_size) + amount = amount / 2 + amount; + if(amount < n) + amount = n; // overflow guardAlso applies to: 199-222
include/boost/capy/io/any_write_sink.hpp (1)
576-624:writeandwrite_eof(CB)share an identical drain loop.Lines 583–594 and 606–617 are the same consume-all loop over
write_some_. Consider extracting it into a private helper (e.g.,write_all_) that both call, to avoid the duplication and the risk of them drifting apart.Sketch
+// private helper +inline io_task<std::size_t> +any_write_sink::write_all_(buffer_param_base& bp) +{ + std::size_t total = 0; + for(;;) + { + auto bufs = bp.data(); + if(bufs.empty()) + break; + auto [ec, n] = co_await write_some_(bufs); + if(ec) + co_return {ec, total + n}; + bp.consume(n); + total += n; + } + co_return {{}, total}; +}include/boost/capy/io/pull_from.hpp (1)
69-100: Potential unbounded loop whensink.prepare()persistently returns empty.Lines 72-78: if
sink.prepare()always returns an empty span andsink.commit(0)always succeeds without error, this loop never terminates. The same pattern exists in overload 2 (lines 150-161). This is arguably a contract violation by the sink, but a bounded retry or a diagnostic could make this more robust.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
example/stream-pipeline/stream_pipeline.cpp (1)
21-21:⚠️ Potential issue | 🟡 MinorStale comment:
task<>should readio_task<>.Line 21 says the transforms use
task<>coroutines, but they now returnio_task<>.✏️ Suggested fix
-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/push_to.hpp (1)
141-146:⚠️ Potential issue | 🟠 MajorInconsistent error handling between overloads: bytes written but not counted on error.
In the
WriteSinkoverload (lines 79–83),total += nandsource.consume(n)execute before checkingwrite_ec, so the returned total and source state are correct even on partial-write errors.In this
WriteStreamoverload, the early return on line 143 skips bothtotal += nandsource.consume(n). Ifwrite_somewroten > 0bytes but also set an error, the returnedtotalunder-reports and the source position is stale.🔧 Suggested fix: match the WriteSink overload's ordering
auto [write_ec, n] = co_await stream.write_some(bufs); - if(write_ec) - co_return {write_ec, total}; - total += n; source.consume(n); + if(write_ec) + co_return {write_ec, total}; }
🧹 Nitpick comments (2)
include/boost/capy/io/any_buffer_sink.hpp (1)
711-747:write_eofbody is nearly identical towrite— consider extracting a shared helper.Lines 718–740 duplicate the prepare/copy/commit loop from
write(Lines 684–706). The only addition is the trailingcommit_eof()call. A private helper (e.g.,write_all_(buffer_param<CB>&)) returningio_task<std::size_t>could be reused by bothwriteandwrite_eof, reducing maintenance surface.include/boost/capy/io/any_write_sink.hpp (1)
576-624:writeandwrite_eof(CB)share the same loop — consider extracting a helper.Lines 583–594 and 606–617 are the same
write_some_accumulation loop. A private helper could be shared, withwrite_eofappending the EOF signal after the helper returns. Same pattern as noted inany_buffer_sink.hpp.♻️ Sketch: extract shared loop
+// private helper +template<ConstBufferSequence CB> +io_task<std::size_t> +any_write_sink::write_all_(buffer_param<CB>& bp) +{ + std::size_t total = 0; + for(;;) + { + auto bufs = bp.data(); + if(bufs.empty()) + break; + auto [ec, n] = co_await write_some_(bufs); + if(ec) + co_return {ec, total + n}; + bp.consume(n); + total += n; + } + co_return {{}, total}; +} template<ConstBufferSequence CB> io_task<std::size_t> any_write_sink::write(CB buffers) { buffer_param<CB> bp(buffers); - std::size_t total = 0; - for(;;) - { - auto bufs = bp.data(); - if(bufs.empty()) - break; - auto [ec, n] = co_await write_some_(bufs); - if(ec) - co_return {ec, total + n}; - bp.consume(n); - total += n; - } - co_return {{}, total}; + co_return co_await write_all_(bp); } template<ConstBufferSequence CB> io_task<std::size_t> any_write_sink::write_eof(CB buffers) { buffer_param<CB> bp(buffers); - std::size_t total = 0; - for(;;) - { - auto bufs = bp.data(); - if(bufs.empty()) - break; - auto [ec, n] = co_await write_some_(bufs); - if(ec) - co_return {ec, total + n}; - bp.consume(n); - total += n; - } + auto [ec, total] = co_await write_all_(bp); + if(ec) + co_return {ec, total}; auto [ec2] = co_await write_eof(); if(ec2) co_return {ec2, total}; co_return {{}, total}; }
…riteStream Establish the definitive concept refinement hierarchy for Capy's I/O layer. Every ReadSource is now a ReadStream; every WriteSink is now a WriteStream. Algorithms constrained on the base concepts accept both streams and their refined counterparts, unlocking composability across the entire I/O stack. WriteSink drops the awkward write(buffers, bool eof) two-arg overload in favor of the atomic write_eof(buffers)--cleaner semantics, no boolean parameter, no ambiguity. Concept headers, type-erased wrappers, test mocks, concept static_assert tests, and integration tests all updated to match the design documents.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
example/stream-pipeline/stream_pipeline.cpp (1)
20-22:⚠️ Potential issue | 🟡 MinorStale comment: still references
task<>instead ofio_task<>.Line 20 says "The transforms use task<> coroutines for their pull() methods" but both transforms now return
io_task<std::span<const_buffer>>.📝 Proposed fix
-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/push_to.hpp (1)
141-146:⚠️ Potential issue | 🟡 MinorInconsistent error-path accounting between the two
push_tooverloads.In the
WriteSinkoverload (Lines 79-83),total += nandsource.consume(n)execute before the error check, sototalreflects bytes actually written even on error. In thisWriteStreamoverload, the error check at Line 142 returns before updatingtotalor consuming from the source. Ifwrite_somereports both an error and a partial write (n > 0), the returnedtotalwill undercount.This is pre-existing behavior (not introduced by this PR), but now that both overloads share the same
io_task<std::size_t>return type, callers might expect consistent semantics.📝 Suggested fix to match the WriteSink overload's accounting
auto [write_ec, n] = co_await stream.write_some(bufs); + total += n; + source.consume(n); if(write_ec) co_return {write_ec, total}; - - total += n; - source.consume(n);
🧹 Nitpick comments (3)
include/boost/capy/concept/read_source.hpp (1)
26-26: Nit: Brief should explicitly name the concept.Per the concept javadoc guidelines, the brief should contain one sentence naming the concept. Consider:
-/** Concept for types providing complete reads from a data source. +/** The `ReadSource` concept models types providing complete reads from a data source.As per coding guidelines: "Concept javadoc Brief section should contain one sentence naming the concept and stating what a conforming type does."
include/boost/capy/io/any_read_stream.hpp (2)
385-396: Constructing the awaitable insideawait_readycreates a narrow exception-safety gap.If
vt_->await_ready(Line 394) throws afterawaitable_active_is set totrue(Line 392), neitherawait_suspendnorawait_resumewill run, so the constructed awaitable is never destroyed via theawait_resumeguard. The nextread_somecall will overwrite the cached storage without first destroying the old awaitable.This is likely benign in practice (most
await_readyimplementations arenoexcept), but you could defend against it by wrapping theawait_readycall in a try/catch that destroys the awaitable and resets the flag on failure.
234-248: Design divergence:any_read_streaminlines vtable ops whileany_write_streamkeepsawaitable_ops.
any_read_streamnow constructs the awaitable inawait_readyand stores all ops (await_ready,await_suspend,await_resume,destroy_awaitable) directly in the vtable, eliminatingawaitable_ops. Meanwhile,any_write_streamretains theawaitable_opsindirection and constructs the awaitable inawait_suspend.Both approaches work, but the inconsistency may cause confusion for maintainers. Consider aligning the two wrappers to the same pattern in a follow-up.
Also applies to: 272-290
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
include/boost/capy/io/any_write_stream.hpp (1)
304-318:⚠️ Potential issue | 🟠 MajorDestructor does not pass alignment to
operator delete.If the alignment fix above is applied, the delete calls here (and in move-assignment at line 334) must also pass
std::align_val_t. Additionally, the stream storage delete on line 310 has the same pattern —::operator delete(storage_)without size/alignment, though for the stream object this is a separate concern.include/boost/capy/io/any_read_stream.hpp (1)
356-362:⚠️ Potential issue | 🟡 MinorPreallocated storage may be under-aligned for the awaitable.
::operator new(vt_->awaitable_size)allocates with default alignment (__STDCPP_DEFAULT_NEW_ALIGNMENT__), but the awaitable's required alignment (vt_->awaitable_align) could in theory exceed that. Theawaitable_alignfield is stored in the vtable but never used during allocation.If awaitable types always have alignment ≤ default new alignment this is fine in practice, but it would be safer to use the aligned overload.
🛡️ Suggested fix
- cached_awaitable_ = ::operator new(vt_->awaitable_size); + cached_awaitable_ = ::operator new( + vt_->awaitable_size, + std::align_val_t{vt_->awaitable_align});And correspondingly in the destructor / move-assignment
::operator deletecalls:- ::operator delete(cached_awaitable_); + ::operator delete( + cached_awaitable_, + std::align_val_t{vt_->awaitable_align});include/boost/capy/io/any_buffer_source.hpp (1)
373-383:⚠️ Potential issue | 🟠 MajorDestructor and move-assignment don't destroy an in-flight awaitable before freeing
cached_awaitable_.Unlike
any_read_streamwhich checksawaitable_active_before freeing,any_buffer_sourcefreescached_awaitable_without first callingactive_ops_->destroy(...). If the object is destroyed or moved-from while apull()awaitable is live (e.g., coroutine destroyed between suspend and resume), the awaitable's destructor is skipped, potentially leaking resources.🛡️ Suggested fix (destructor)
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }Apply the same pattern in
operator=(any_buffer_source&&).Also applies to: 385-404
include/boost/capy/io/any_read_source.hpp (1)
390-400:⚠️ Potential issue | 🟠 MajorSame destructor/move-assignment issue: in-flight awaitable not destroyed before freeing storage.
Same pattern as
any_buffer_source— if the object is destroyed while an awaitable is active (i.e.,active_ops_ != nullptr), the awaitable destructor is never called. Apply the same fix: checkactive_ops_and callactive_ops_->destroy(cached_awaitable_)before::operator delete.Also applies to: 402-421
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/any_write_stream.hpp`:
- Around line 250-251: The vtable stores awaitable_align but allocation uses
::operator new(vt_->awaitable_size) which can violate alignment for over-aligned
Awaitable; fix by using the stored alignment when allocating and deallocating
the cached awaitable (use the aligned allocation form with vt_->awaitable_align
for the allocation sites that use vt_->awaitable_size and pass the same
alignment to ::operator delete for cached_awaitable_ deallocations), or if
alignment isn't needed remove awaitable_align from the vtable and related logic;
update references to vt_, awaitable_size, awaitable_align, and cached_awaitable_
accordingly so placement new is safe.
🧹 Nitpick comments (4)
include/boost/capy/io/any_write_stream.hpp (2)
347-370: Owning constructor: stream storage allocation doesn't account for alignment either.Line 363 uses
::operator new(sizeof(S))butSmay have alignment greater than the default. Consider::operator new(sizeof(S), std::align_val_t{alignof(S)})(and matching delete in the guard and destructor). This is an existing pattern, not introduced by this PR, so lower priority.
185-209:write_somejavadoc is missing several required sections.Per the coding guidelines for async/awaitable function javadoc, the following are absent: error conditions (
@lilist of notableerror_codeconditions), cancellation support documentation,@throws,@par Examplewith@codeblocks, and@see. Consider adding these to stay consistent with project documentation standards, especially@par Exampleand@seewhich help discoverability.As per coding guidelines: "Document error conditions in async/awaitable function javadoc" and "Include
@parExample in async/awaitable function javadoc with two or three@codeblocks."include/boost/capy/buffers/buffer_param.hpp (1)
143-146: Union-based storage requires trivial destructibility ofbuffer_type.The anonymous union suppresses automatic destruction of
arr_elements, and no explicit destructor calls are made on consumed/abandoned elements. This is correct only ifbuffer_type(const_buffer/mutable_buffer) is trivially destructible. Consider adding astatic_assertto guard this invariant.🛡️ Suggested guard
+ static_assert( + std::is_trivially_destructible_v<buffer_type>, + "buffer_type must be trivially destructible for union storage"); union { int dummy_; buffer_type arr_[detail::max_iovec_]; };include/boost/capy/io/any_read_source.hpp (1)
370-377:max_awaitable_size/max_awaitable_aligncomputation is correct.Using
std::maxwould be slightly more idiomatic than ternary, but this is trivially correct at compile time.
| std::size_t awaitable_size; | ||
| std::size_t awaitable_align; |
There was a problem hiding this comment.
awaitable_align is stored in the vtable but never used during allocation.
Lines 367 and 378 allocate with ::operator new(vt_->awaitable_size), which only guarantees default new-alignment (typically 16 bytes). If Awaitable is over-aligned, the placement new at line 274 is undefined behavior. Either use the stored alignment:
Proposed fix
- cached_awaitable_ = ::operator new(vt_->awaitable_size);
+ cached_awaitable_ = ::operator new(
+ vt_->awaitable_size,
+ std::align_val_t{vt_->awaitable_align});…and correspondingly update all ::operator delete(cached_awaitable_) calls (lines 316, 334) to pass the alignment, or remove awaitable_align from the vtable if it is intentionally unused.
Also applies to: 367-367, 378-378
🤖 Prompt for AI Agents
In `@include/boost/capy/io/any_write_stream.hpp` around lines 250 - 251, The
vtable stores awaitable_align but allocation uses ::operator
new(vt_->awaitable_size) which can violate alignment for over-aligned Awaitable;
fix by using the stored alignment when allocating and deallocating the cached
awaitable (use the aligned allocation form with vt_->awaitable_align for the
allocation sites that use vt_->awaitable_size and pass the same alignment to
::operator delete for cached_awaitable_ deallocations), or if alignment isn't
needed remove awaitable_align from the vtable and related logic; update
references to vt_, awaitable_size, awaitable_align, and cached_awaitable_
accordingly so placement new is safe.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/any_write_sink.hpp`:
- Around line 354-358: The vtable field awaitable_align is computed but not
honored when allocating cached_awaitable_ (allocations invoked via ::operator
new(vt_->awaitable_size)); change those allocations that create storage for the
eof_awaitable_ops (where construct_eof_awaitable is later placement-new into
cached_awaitable_) to use the aligned allocation form ::operator
new(vt_->awaitable_size, std::align_val_t{vt_->awaitable_align}) and
correspondingly use the aligned deallocation ::operator delete(ptr,
std::align_val_t{vt_->awaitable_align}) (or the sized aligned delete overload)
in the destructor and in move-assignment cleanup; ensure all paths that free or
reallocate cached_awaitable_ use the same alignment-aware delete to avoid UB for
over-aligned awaitables.
🧹 Nitpick comments (3)
include/boost/capy/buffers/buffer_param.hpp (1)
143-146: Add astatic_assertto guard the union/placement-new pattern.The anonymous union avoids default-constructing
buffer_typeelements, which is a nice optimization. However, the correctness of skipping explicit destructor calls depends onbuffer_typebeing trivially destructible. Astatic_assertwould prevent silent UB if this assumption ever breaks.Suggested guard
+ static_assert( + std::is_trivially_destructible_v<buffer_type>, + "buffer_type must be trivially destructible for the union optimization"); union { int dummy_; buffer_type arr_[detail::max_iovec_]; };include/boost/capy/io/any_write_sink.hpp (2)
241-268: Javadoc references adetail::symbol.Line 245 mentions
@ref detail::max_iovec_in public-facing documentation. As per coding guidelines, detail/implementation symbols should not be documented in javadoc. Consider rephrasing to describe the limit without naming the internal constant (e.g., "the implementation's maximum scatter/gather buffer count").
192-287: Async function javadoc is missing several sections per guidelines.The new
write_some,write,write_eof(CB), andwrite_eof()javadocs cover brief, extended description,@param,@return, immediate-completion, and preconditions — but the coding guidelines for async/awaitable functions also call for@throws,@see,@par Example, cancellation support, error conditions, and concurrency/overlap documentation. These can be deferred, but noting for completeness.
| eof_awaitable_ops const* (*construct_eof_awaitable)( | ||
| void* sink, | ||
| void* storage); | ||
| std::size_t awaitable_size; | ||
| std::size_t awaitable_align; |
There was a problem hiding this comment.
awaitable_align is computed but never used in allocations.
The vtable stores awaitable_align (and max4 carefully computes it across all four awaitable types), but the cached_awaitable_ allocations at Lines 576 and 587 use ::operator new(vt_->awaitable_size) without passing the alignment. If any wrapped sink produces an over-aligned awaitable, placement-new into this storage is undefined behavior.
Consider using ::operator new(vt_->awaitable_size, std::align_val_t{vt_->awaitable_align}) and the corresponding ::operator delete overload in the destructor and move-assignment.
🤖 Prompt for AI Agents
In `@include/boost/capy/io/any_write_sink.hpp` around lines 354 - 358, The vtable
field awaitable_align is computed but not honored when allocating
cached_awaitable_ (allocations invoked via ::operator new(vt_->awaitable_size));
change those allocations that create storage for the eof_awaitable_ops (where
construct_eof_awaitable is later placement-new into cached_awaitable_) to use
the aligned allocation form ::operator new(vt_->awaitable_size,
std::align_val_t{vt_->awaitable_align}) and correspondingly use the aligned
deallocation ::operator delete(ptr, std::align_val_t{vt_->awaitable_align}) (or
the sized aligned delete overload) in the destructor and in move-assignment
cleanup; ensure all paths that free or reallocate cached_awaitable_ use the same
alignment-aware delete to avoid UB for over-aligned awaitables.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/boost/capy/io/any_write_sink.hpp (1)
521-531:⚠️ Potential issue | 🔴 CriticalMissing destruction of active awaitables in destructor and move-assignment.
Unlike
any_write_streamwhich checksawaitable_active_and callsvt_->destroy_awaitable(cached_awaitable_)before freeing memory,any_write_sinkfreescached_awaitable_without destroying any in-flight awaitable. If the wrapper is destroyed or moved-from while an operation is suspended (e.g. coroutine destroyed during cancellation), the awaitable object incached_awaitable_will not have its destructor called — leaking resources or causing UB.Proposed fix for destructor
inline any_write_sink::~any_write_sink() { if(storage_) { vt_->destroy(sink_); ::operator delete(storage_); } if(cached_awaitable_) + { + if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); + else if(active_eof_ops_) + active_eof_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + } }Proposed fix for move-assignment
if(cached_awaitable_) + { + if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); + else if(active_eof_ops_) + active_eof_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }Also applies to: 533-553
🧹 Nitpick comments (2)
include/boost/capy/io/any_write_stream.hpp (1)
236-250: vtable fielddestroynaming is ambiguous.The vtable has both
destroy_awaitable(line 246) anddestroy(line 249). The latter destroys the stream object, but the name alone doesn't convey that. Considerdestroy_streamfor clarity, consistent with any_write_sink's similar pattern.include/boost/capy/io/any_write_sink.hpp (1)
593-638: Four nearly identical internal awaitable structs — consider extracting a helper.
write_some_,write_,write_eof_buffers_, andwrite_eofeach define an inner awaitable with the same structure: store self + optional buffers, construct via vtable inawait_suspend, guard-destroy inawait_resume. The only variation is which vtable constructor is called and the return type (io_result<std::size_t>vsio_result<>).A parameterized helper (e.g., templated on the ops type and constructor function pointer) could eliminate this repetition. Not urgent given the code is correct, but worth considering as the pattern grows.
Also applies to: 640-685, 687-732, 734-780
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/any_write_sink.hpp`:
- Around line 610-655: The private method any_write_sink::write_some_ is unused
and should be removed: delete the write_some_ function definition (and its
declaration if present) from any_write_sink, since the public write_some
template and methods write and write_eof do not call it; after removal, run a
full build and tests to ensure there are no remaining references to
any_write_sink::write_some_ and clean up any related private members (e.g.,
cached_awaitable_ usage) only if they are now unused.
In `@include/boost/capy/io/any_write_stream.hpp`:
- Around line 389-395: The awaitable constructor for any_write_stream (the
function named awaitable that takes any_write_stream* self, CB const& buffers)
is incorrectly marked noexcept; remove the noexcept specifier to match the
equivalent awaitable in any_read_source::read_some so both awaitable
implementations have the same exception specification and behavior.
🧹 Nitpick comments (7)
include/boost/capy/io/any_read_stream.hpp (2)
391-402: Inconsistent awaitable construction placement vs.any_write_streamandany_read_source.In this file, the underlying awaitable is constructed inside
await_ready()(lines 394-398), whereas inany_write_stream::write_someandany_read_source::read_somethe construction happens insideawait_suspend(). Both are functionally correct, but the divergence makes the codebase harder to reason about uniformly.Consider aligning on one pattern across all four type-erased wrappers for maintainability.
185-213: Async function javadoc is missing several sections required by coding guidelines.The
read_somedoc lacks@throws, cancellation support, error conditions, concurrency/overlap, and@see. As per coding guidelines, async/awaitable function javadoc should document cancellation support (stop_token or cancel()), error conditions with notable error_code values, and include@seecross-references.include/boost/capy/io/any_read_source.hpp (2)
374-381: Consider usingstd::maxinstead of manual ternaries.Minor readability improvement —
std::maxis clearer andconstexpr-compatible since C++14.Proposed fix
- static constexpr std::size_t max_awaitable_size = - sizeof(ReadSomeAwaitable) > sizeof(ReadAwaitable) - ? sizeof(ReadSomeAwaitable) - : sizeof(ReadAwaitable); - static constexpr std::size_t max_awaitable_align = - alignof(ReadSomeAwaitable) > alignof(ReadAwaitable) - ? alignof(ReadSomeAwaitable) - : alignof(ReadAwaitable); + static constexpr std::size_t max_awaitable_size = + std::max(sizeof(ReadSomeAwaitable), sizeof(ReadAwaitable)); + static constexpr std::size_t max_awaitable_align = + std::max(alignof(ReadSomeAwaitable), alignof(ReadAwaitable));
187-214: Asyncread_somejavadoc is missing several guideline-required sections.Missing: cancellation support, error conditions (notable
error_codevalues),@throws, concurrency/overlap,@par Example, and@see. Same applies toreadat lines 216-250. As per coding guidelines, async/awaitable function javadoc should include these sections.include/boost/capy/io/any_write_stream.hpp (1)
185-213: Asyncwrite_somejavadoc is missing several guideline-required sections.Missing: cancellation support, error conditions,
@throws, concurrency/overlap,@par Example, and@see. As per coding guidelines, async/awaitable function javadoc should document these for each awaitable function.include/boost/capy/io/any_write_sink.hpp (2)
192-293: Async function javadocs forwrite_some,write,write_eof(buffers), andwrite_eof()are missing several guideline-required sections.All four are missing: cancellation support, error conditions,
@throws, concurrency/overlap,@par Example, and@see. As per coding guidelines, each async/awaitable function javadoc should include these sections.
492-499:max4helper is fine but could usestd::maxwith an initializer list.Stylistic nit for consistency with the
std::maxsuggestion on the read-source file.Proposed alternative
- static constexpr std::size_t max4( - std::size_t a, std::size_t b, - std::size_t c, std::size_t d) noexcept - { - std::size_t ab = a > b ? a : b; - std::size_t cd = c > d ? c : d; - return ab > cd ? ab : cd; - } - static constexpr std::size_t max_awaitable_size = - max4(sizeof(WriteSomeAwaitable), - sizeof(WriteAwaitable), - sizeof(WriteEofBuffersAwaitable), - sizeof(EofAwaitable)); + std::max({sizeof(WriteSomeAwaitable), + sizeof(WriteAwaitable), + sizeof(WriteEofBuffersAwaitable), + sizeof(EofAwaitable)}); static constexpr std::size_t max_awaitable_align = - max4(alignof(WriteSomeAwaitable), - alignof(WriteAwaitable), - alignof(WriteEofBuffersAwaitable), - alignof(EofAwaitable)); + std::max({alignof(WriteSomeAwaitable), + alignof(WriteAwaitable), + alignof(WriteEofBuffersAwaitable), + alignof(EofAwaitable)});
| inline auto | ||
| any_write_sink::write_some_( | ||
| std::span<const_buffer const> buffers, | ||
| bool eof) | ||
| std::span<const_buffer const> buffers) | ||
| { | ||
| struct awaitable | ||
| { | ||
| any_write_sink* self_; | ||
| std::span<const_buffer const> buffers_; | ||
|
|
||
| bool | ||
| await_ready() const noexcept | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| coro | ||
| await_suspend(coro h, executor_ref ex, std::stop_token token) | ||
| { | ||
| self_->active_write_ops_ = self_->vt_->construct_write_some_awaitable( | ||
| self_->sink_, | ||
| self_->cached_awaitable_, | ||
| buffers_); | ||
|
|
||
| if(self_->active_write_ops_->await_ready(self_->cached_awaitable_)) | ||
| return h; | ||
|
|
||
| return self_->active_write_ops_->await_suspend( | ||
| self_->cached_awaitable_, h, ex, token); | ||
| } | ||
|
|
||
| io_result<std::size_t> | ||
| await_resume() | ||
| { | ||
| struct guard { | ||
| any_write_sink* self; | ||
| ~guard() { | ||
| self->active_write_ops_->destroy(self->cached_awaitable_); | ||
| self->active_write_ops_ = nullptr; | ||
| } | ||
| } g{self_}; | ||
| return self_->active_write_ops_->await_resume( | ||
| self_->cached_awaitable_); | ||
| } | ||
| }; | ||
| return awaitable{this, buffers}; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'write_some_\(' --type=cppRepository: cppalliance/capy
Length of output: 234
🏁 Script executed:
cat -n include/boost/capy/io/any_write_sink.hpp | sed -n '315,325p'Repository: cppalliance/capy
Length of output: 351
Remove the unused write_some_ method — it has no callers in the codebase.
The private method write_some_ (lines 610–655) is defined but never invoked. The public write_some template (line 799) has its own inline awaitable and does not delegate to write_some_, and neither write nor write_eof call it.
🤖 Prompt for AI Agents
In `@include/boost/capy/io/any_write_sink.hpp` around lines 610 - 655, The private
method any_write_sink::write_some_ is unused and should be removed: delete the
write_some_ function definition (and its declaration if present) from
any_write_sink, since the public write_some template and methods write and
write_eof do not call it; after removal, run a full build and tests to ensure
there are no remaining references to any_write_sink::write_some_ and clean up
any related private members (e.g., cached_awaitable_ usage) only if they are now
unused.
|
GCOVR code coverage report https://133.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-07 07:25:15 UTC |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
include/boost/capy/buffers/buffer_param.hpp (1)
150-161:⚠️ Potential issue | 🟡 Minor
refill()silently skips empty buffers — verify this is intentional and understood by callers.The loop skips zero-length buffers (
buf.size() > 0guard at line 158), which meansmore()could returntrueeven when all remaining buffers are empty, causingdata()to eventually return an empty span. This is internally consistent (the loop will drain the remaining empty buffers andsize_stays 0), but callers relying onmore()as a "there's still data" signal rather than "there are still unvisited buffer entries" could be surprised.If this is the intended semantic, a brief
@noteonmore()clarifying that it reflects unvisited entries (not necessarily non-empty data) would help prevent misuse.example/stream-pipeline/stream_pipeline.cpp (1)
20-22:⚠️ Potential issue | 🟡 MinorStale comment:
task<>→io_task<>.The file-level comment still says "task<> coroutines" but the transforms now use
io_task<>.Suggested fix
-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/push_to.hpp (1)
140-145:⚠️ Potential issue | 🟠 MajorInconsistent
totalaccounting on write error between overloads.In the WriteSink overload (lines 79-83),
total += nandsource.consume(n)happen before the write error check, sototalreflects bytes actually written even on error. Here in the WriteStream overload,total += nhappens after the error check (line 144), so ifwrite_somereturns a non-zeronalongside a write error, those bytes are silently dropped from the reported total.This also means the docstring promise ("n is the total number of bytes transferred before the error") is inaccurate for this overload if partial writes with errors are possible.
Suggested fix: match the WriteSink overload's ordering
auto [write_ec, n] = co_await stream.write_some(bufs); + total += n; + source.consume(n); if(write_ec) co_return {write_ec, total}; - - total += n; - source.consume(n);include/boost/capy/io/any_buffer_source.hpp (1)
373-383:⚠️ Potential issue | 🟠 MajorMissing active awaitable destruction in destructor — resource leak if destroyed mid-operation.
Unlike
any_read_source(which checksactive_ops_and callsactive_ops_->destroy(cached_awaitable_)before freeing),any_buffer_source's destructor freescached_awaitable_without destroying the live awaitable object. If the wrapper is destroyed while apulloperation is in-flight (e.g., the owningio_taskis dropped), the awaitable incached_awaitable_won't have its destructor called.The same issue exists in the move-assignment operator at Lines 395–396.
Proposed fix for destructor
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }Proposed fix for move-assignment
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }
🧹 Nitpick comments (8)
include/boost/capy/buffers/buffer_param.hpp (1)
143-161: Consider adding astatic_assertfor trivial destructibility.The union trick to defer construction is well-established and appropriate here. However,
refill()overwrites elements without destroying them, and there's no user-defined destructor forbuffer_param. This is correct only becauseconst_buffer/mutable_bufferare trivially destructible, but nothing enforces that at compile time.A
static_assertwould prevent a silent resource leak ifbuffer_typeever changes or if the template is instantiated with a non-trivial buffer sequence type:Suggested guard
class buffer_param { public: /// The buffer type (const_buffer or mutable_buffer) using buffer_type = std::conditional_t< MakeConst, const_buffer, capy::buffer_type<BS>>; + static_assert( + std::is_trivially_destructible_v<buffer_type>, + "buffer_type must be trivially destructible for union storage"); private:example/stream-pipeline/stream_pipeline.cpp (1)
246-281:transfer()EOF flow looks correct, minor nit on structured binding consistency.The EOF-break +
write_eof()pattern is sound. One small observation: the rest of the function uses structured bindings (auto [ec, bufs]), but line 276 uses a named variableeof_resultand then accesses.ec. Not a problem—just a slight inconsistency. You could useauto [eof_ec] = co_await sink.write_eof();to match the style.Optional: consistent structured binding
- io_result<> eof_result = co_await sink.write_eof(); - if (eof_result.ec) - throw std::system_error(eof_result.ec); + auto [eof_ec] = co_await sink.write_eof(); + if (eof_ec) + throw std::system_error(eof_ec);include/boost/capy/io/push_to.hpp (2)
40-40:@returnformat doesn't match coding guideline.The guideline specifies: "An awaitable yielding
(error_code, T1, T2, T3)" — usingerror_codewithout thestd::prefix and starting with "An awaitable yielding". Both overloads'@returncurrently read "A task that yields(std::error_code, std::size_t)".Suggested fix (both overloads)
- `@return` A task that yields `(std::error_code, std::size_t)`. + `@return` An awaitable yielding `(error_code,std::size_t)`.As per coding guidelines:
Document io_result<T1,T2,T3> return types with the format: 'An awaitable yielding (error_code, T1, T2, T3)' - include only type names without internal spaces.Also applies to: 104-104
135-136: EOF return uses{}— consider explicitstd::error_code{}for clarity.Line 136 returns
co_return {{}, total}to indicate success on EOF. While{}works due to aggregate initialization, the WriteSink overload on line 74 uses the namedeof_ecfromwrite_eof(). Usingstd::error_code{}here would make the "no error" intent more explicit and consistent with the rest of the codebase.include/boost/capy/io/any_read_source.hpp (1)
473-527: Inconsistentawait_ready/ construction pattern vs.any_read_stream.In
any_read_stream::read_some, the inner awaitable is constructed insideawait_ready, while here it's constructed insideawait_suspend. Both work correctly, but the divergence means the empty-buffer fast path is handled differently:
- Here:
await_readyreturnstruefor empty buffers →await_resumereturns{{}, 0}without ever constructing an inner awaitable.any_read_stream:await_readyalways constructs the inner awaitable and delegates tovt_->await_ready.Both are correct, but the inconsistency may make maintenance harder. Not blocking since the class responsibilities differ (stream has no empty-buffer fast path).
include/boost/capy/io/write_now.hpp (3)
32-72: Class documentation should note the destruction precondition.The class javadoc documents the precondition about one outstanding operation at a time, but doesn't state that all operations must have completed before
write_nowis destroyed. Since~write_nowunconditionally freescached_frame_(line 262-263), destroying the object while anop_typestill holds a handle into that memory would be use-after-free.Consider adding a
@noteor extending the@par Preconditionssection:Suggested addition
Only one operation may be outstanding at a time. A new call to `operator()` must not be made until the previous operation has completed (i.e., the returned awaitable has been fully consumed). + The returned awaitable must be fully consumed before this object + is destroyed.
279-312: Async/awaitable javadoc is missing several sections required by the coding guidelines.Per the project's documentation guidelines for async/awaitable functions, the
operator()javadoc is missing:
- @throws — should document precondition violations or state that no exceptions are thrown during normal operation.
- Cancellation support — should state whether the operation supports cancellation via
stop_tokenor the stream'scancel()member.- Concurrency/overlap — should state which operations may be simultaneously in flight.
- Completion conditions — should list conditions as a bulleted
@lilist (e.g., "The supplied buffers are fully written", "An error occurs").Also on line 295, the
@returnformat has(error_code,std::size_t)— the guideline specifies spaces after commas:(error_code, std::size_t).As per coding guidelines: "Document
io_result<T1,T2,T3>return types with the format: 'An awaitable yielding(error_code, T1, T2, T3)' - include only type names without internal spaces" and the async/awaitable function javadoc requirements.
73-79: Consider documenting or asserting the single-operation invariant at runtime (debug builds).
cached_frame_is reused under the assumption that no twoop_typecoroutines overlap. A debug-only assertion (e.g., a boolean flag set/cleared around the operation) would catch misuse early during development. This is entirely optional and low priority.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
example/stream-pipeline/stream_pipeline.cpp (1)
20-22:⚠️ Potential issue | 🟡 MinorStale comment:
task<>should beio_task<>.The file-level overview still references
task<>coroutines, but thepull()methods now returnio_task<>.-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/push_to.hpp (1)
140-146:⚠️ Potential issue | 🟡 MinorInconsistent
totalaccounting on write error between the two overloads.In the Sink overload (lines 79–83),
total += nis done before checkingwrite_ec, so the returned total includes bytes from the failing write. In the Stream overload here,total += nis done after thewrite_eccheck (lines 144–145), so partial bytes from a failingwrite_someare silently lost from the returned total.Pick one convention. The Sink variant's approach (always account for bytes that were actually written) is typically more useful to callers.
Suggested fix — match Sink overload's ordering
auto [write_ec, n] = co_await stream.write_some(bufs); + total += n; + source.consume(n); if(write_ec) co_return {write_ec, total}; - - total += n; - source.consume(n); }include/boost/capy/io/any_buffer_source.hpp (1)
551-561:⚠️ Potential issue | 🟠 MajorDestructor does not destroy in-flight awaitables, unlike
any_read_source.In
any_read_source::~any_read_source()(lines 394–407 of that file), the destructor checksactive_ops_and callsactive_ops_->destroy(cached_awaitable_)before freeingcached_awaitable_. Here, neitheractive_ops_noractive_read_ops_is checked/destroyed before::operator delete(cached_awaitable_). If the wrapper is destroyed while an operation is mid-flight (e.g., coroutine abandoned afterawait_suspendbut beforeawait_resume), the placement-new'd awaitable will not be properly destructed.The same issue applies to the move assignment operator at lines 563–583.
Proposed fix for destructor
inline any_buffer_source::~any_buffer_source() { if(storage_) { vt_->destroy(source_); ::operator delete(storage_); } if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_read_ops_) + active_read_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + } }include/boost/capy/io/any_buffer_sink.hpp (1)
725-735:⚠️ Potential issue | 🟠 MajorDestructor does not destroy in-flight awaitables, unlike
any_write_sink.Same issue as flagged for
any_buffer_source.hpp: the destructor freescached_awaitable_without checking/destroying any active awaitable throughactive_ops_oractive_write_ops_. Compare withany_write_sink::~any_write_sink()(lines 526–541 of that file) which properly checks bothactive_write_ops_andactive_eof_ops_.The move assignment operator at lines 737–757 has the same gap.
Proposed fix for destructor
inline any_buffer_sink::~any_buffer_sink() { if(storage_) { vt_->destroy(sink_); ::operator delete(storage_); } if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + } }
🤖 Fix all issues with AI agents
In `@example/stream-pipeline/stream_pipeline.cpp`:
- Around line 73-74: The inline comment for BufferSource::pull (and the
duplicate at line 172) is stale — it says "returns task<>" but the function
signature returns io_task<std::span<const_buffer>>; update the comment text to
reflect io_task<> (e.g., "returns io_task<> to enable co_await on upstream") so
the comment matches the actual return type and keep similar wording for both
occurrences to avoid future drift; locate comments near the BufferSource::pull
declaration and the duplicate on line 172 and replace "task<>" with "io_task<>".
🧹 Nitpick comments (7)
example/stream-pipeline/stream_pipeline.cpp (1)
47-132: The two transform classes are nearly identical — consider extracting the shared scaffolding.
uppercase_transformandline_numbering_transformshare the sameconsume()logic, the samepull()skeleton (cached-data check → exhausted check → upstream pull → eof/error handling → consume → return), and differ only in the inner transform loop (lines 111-121 vs 210-226). A small base class or CRTP mixin could eliminate the duplication while keeping the example readable.Not blocking for a demo, but worth noting if this pattern is intended to be copied by users.
include/boost/capy/buffers/buffer_array.hpp (1)
202-203: Consider constrainingIteratorto prevent greedy template matching.The unconstrained
Iteratortemplate could match non-iterator arguments (e.g., two integers), producing hard-to-read errors deep in the body rather than at overload resolution. A minimal concept or SFINAE guard (e.g.,requires std::input_iterator<Iterator>or requiring*firstto be convertible tovalue_type) would improve diagnostics and prevent accidental hijacking of other two-argument calls.This mirrors the
requiresclauses already present on the buffer-sequence constructors (lines 142–144).Suggested constraint
template<class Iterator> + requires std::convertible_to<decltype(*std::declval<Iterator&>()), value_type> buffer_array(Iterator first, Iterator last) noexceptApply the same constraint to the
std::in_place_toverload on line 229.include/boost/capy/buffers/buffer_param.hpp (1)
143-146: Union-based storage lacks explicit destruction of active elements.With the switch from a plain array to a union + placement-new, the compiler-generated destructor will not destroy the active
arr_elements. Similarly,refill()resetssize_to 0 (line 154) without destroying existing elements first.For
const_buffer/mutable_buffer(trivially destructible POD-like types) this is harmless in practice. However,buffer_array— which uses the same union pattern — has an explicit destructor (line 249–253 of buffer_array.hpp) that properly destroys active elements. For consistency and defensive correctness, consider adding a destructor here as well.Suggested destructor and refill cleanup
+ ~buffer_param() + { + for(std::size_t i = pos_; i < size_; ++i) + arr_[i].~buffer_type(); + } + void refill() { + for(std::size_t i = pos_; i < size_; ++i) + arr_[i].~buffer_type(); pos_ = 0; size_ = 0;Also applies to: 150-161
include/boost/capy/io/any_read_stream.hpp (1)
384-389: Consider addingif constexprfast path for span as noted in the VFALCO comment.The TODO comment on lines 384-385 notes that a
spancould bypass thebuffer_arraycopy. This would avoid copying buffer descriptors for the common case where the caller already provides a span. Low priority but worth tracking.include/boost/capy/io/write_now.hpp (1)
196-216: Frame-cachingoperator new/operator delete— subtle but correct.The custom allocator reuses
write_now::cached_frame_for the coroutine frame.operator deleteis intentionally a no-op since the memory is owned by thewrite_nowinstance. Key invariant: only oneop_typecan be alive at a time (enforced by the precondition on line 59). If violated, the second call'soperator newwould reuse memory still occupied by the first coroutine — undefined behavior.The precondition is documented but not enforced programmatically. An assertion could catch misuse in debug builds.
Optional debug assertion
static void* operator new( std::size_t size, write_now& self, auto&) { + assert(!self.cached_frame_ || /* frame is not in use by a live coroutine */); if(self.cached_frame_ && self.cached_size_ >= size)(Enforcing this fully would require an additional
in_use_flag onwrite_now.)include/boost/capy/io/any_read_source.hpp (1)
394-407: Alignment not honored in::operator newcalls.Lines 457 and 468 allocate
cached_awaitable_with::operator new(vt_->awaitable_size)but ignorevt_->awaitable_align. If any wrapped source produces an over-aligned awaitable, placement-new into this storage is UB. The same applies to the::operator deletecalls in the destructor and move assignment.This matches the pattern flagged in
any_write_sink.hppandany_write_stream.hpp. Consider using::operator new(vt_->awaitable_size, std::align_val_t{vt_->awaitable_align})and the corresponding aligned::operator deleteconsistently across all type-erased wrappers.include/boost/capy/io/any_write_stream.hpp (1)
384-436: Awaitable construction happens inawait_suspend, notawait_ready— differs fromany_read_stream.In
any_read_stream, the awaitable is constructed inawait_ready(per the snippet at lines 390–401 of that file), which allows immediate completion without ever suspending. Here,await_readyonly checks for empty buffers; the underlying awaitable is constructed inawait_suspend. This means non-empty buffers always enterawait_suspendeven if the underlying stream is immediately ready (it returnshto resume, but the coroutine still technically suspends and resumes).This is functionally correct but creates an asymmetry with the read side. Consider aligning the patterns if consistency matters.
| // BufferSource::pull - returns task<> to enable co_await on upstream | ||
| task<io_result<std::span<const_buffer>>> | ||
| io_task<std::span<const_buffer>> |
There was a problem hiding this comment.
Stale comment: task<> → io_task<>.
Same staleness as the file overview — the inline comment on line 73 says "returns task<>" but the signature is io_task<std::span<const_buffer>>. Line 172 has the identical issue.
- // BufferSource::pull - returns task<> to enable co_await on upstream
+ // BufferSource::pull - returns io_task<> to enable co_await on upstream🤖 Prompt for AI Agents
In `@example/stream-pipeline/stream_pipeline.cpp` around lines 73 - 74, The inline
comment for BufferSource::pull (and the duplicate at line 172) is stale — it
says "returns task<>" but the function signature returns
io_task<std::span<const_buffer>>; update the comment text to reflect io_task<>
(e.g., "returns io_task<> to enable co_await on upstream") so the comment
matches the actual return type and keep similar wording for both occurrences to
avoid future drift; locate comments near the BufferSource::pull declaration and
the duplicate on line 172 and replace "task<>" with "io_task<>".
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
example/stream-pipeline/stream_pipeline.cpp (1)
21-22:⚠️ Potential issue | 🟡 MinorStale file-overview comment:
task<>→io_task<>.Line 21 still says "task<> coroutines" but
pull()now returnsio_task<std::span<const_buffer>>.-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/any_read_source.hpp (1)
435-469:⚠️ Potential issue | 🟡 MinorAlignment not used during awaitable storage allocation.
Both the owning constructor (line 457) and the reference constructor (line 468) allocate
cached_awaitable_via::operator new(vt_->awaitable_size)without passing the storedawaitable_align. The default alignment (__STDCPP_DEFAULT_NEW_ALIGNMENT__, typically 16 bytes) suffices for most awaitable types, but if a concreteReadSomeAwaitableorReadAwaitablehas over-aligned members, this would be UB.This is a pre-existing pattern shared with
any_read_stream, so it may be intentional. If you want to be defensive:Proposed fix (applies to both constructors)
- cached_awaitable_ = ::operator new(vt_->awaitable_size); + cached_awaitable_ = ::operator new( + vt_->awaitable_size, + std::align_val_t(vt_->awaitable_align));And correspondingly in the destructor/move-assignment
::operator deletecalls:- ::operator delete(cached_awaitable_); + ::operator delete( + cached_awaitable_, + std::align_val_t(vt_->awaitable_align));include/boost/capy/io/any_buffer_source.hpp (2)
551-561:⚠️ Potential issue | 🟠 MajorDestructor does not destroy active awaitables before freeing
cached_awaitable_.If the wrapper is destroyed while a coroutine is suspended mid-operation (e.g., between
await_suspendandawait_resume), the awaitable placed intocached_awaitable_is never properly destroyed. Sibling classes (any_write_sink,any_read_stream) handle this by checkingactive_ops_/active_write_ops_and calling the appropriatedestroybefore freeing the memory. The same pattern is missing here for bothactive_ops_andactive_read_ops_.Proposed fix
inline any_buffer_source::~any_buffer_source() { if(storage_) { vt_->destroy(source_); ::operator delete(storage_); } if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_read_ops_) + active_read_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + } }
563-583:⚠️ Potential issue | 🟠 MajorMove-assignment has the same missing active-awaitable destruction.
Same issue as the destructor: when
cached_awaitable_is freed on line 574, any in-flight awaitable is leaked. Apply the same guard pattern.Proposed fix
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_read_ops_) + active_read_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }include/boost/capy/io/any_buffer_sink.hpp (2)
725-735:⚠️ Potential issue | 🟠 MajorDestructor does not destroy active awaitables before freeing
cached_awaitable_.Same issue as
any_buffer_source: if the wrapper is destroyed while a coroutine is suspended mid-operation, the awaitable object incached_awaitable_is never properly destructed. Bothactive_ops_andactive_write_ops_need to be checked.Proposed fix
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }
737-757:⚠️ Potential issue | 🟠 MajorMove-assignment also missing active-awaitable destruction.
Same pattern—
cached_awaitable_is freed on line 748 without destroying any in-flight awaitable.Proposed fix
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }
🧹 Nitpick comments (2)
include/boost/capy/io/write_now.hpp (1)
291-324: Operator() javadoc is missing several sections required by the async/awaitable function guidelines.Per the coding guidelines for async/awaitable functions, the following are absent:
- Completion conditions as a bulleted
@lilist (e.g., "The entire buffer sequence is written", "An error occurs", "The operation was canceled").- Concurrency and overlap section stating that only one operation may be outstanding (already on the class, but the per-function doc should reiterate which operations may be simultaneously in flight).
- Cancellation support — whether the operation supports cancellation via
stop_tokenor the stream'scancel(), and that the resulting error compares equal tocapy::cond::canceled.@throws— state precondition violations or that no exceptions are thrown during normal operation.- Multiple
@codeexamples — guidelines ask for two or three blocks (happy path with destructuring, error handling, cancellation).These can be addressed in a follow-up commit. As per coding guidelines: "Document completion conditions in async/awaitable function javadoc using bulleted
@lilist", "Document cancellation support in async/awaitable function javadoc", "Include@parExample in async/awaitable function javadoc with two or three@codeblocks showing different usage patterns".include/boost/capy/io/any_read_source.hpp (1)
473-527:read_someimmediate-completion path differs fromany_read_stream's approach.In
any_read_stream::read_some, the inner awaitable is constructed inawait_ready, so when the inner awaitable is immediately ready, the coroutine avoids suspension entirely. Here,await_readyonly checks for empty buffers; the inner awaitable is constructed inawait_suspend, which means even data-ready sources incur a suspend→resume round-trip (returningh).This may be intentional (e.g., to defer construction cost), but if immediate completion performance matters for sources, consider aligning with the
any_read_streampattern.
Redesign every type-erasing I/O wrapper (any_buffer_sink, any_buffer_source, any_read_source, any_write_sink, any_read_stream, any_write_stream) around a new split-vtable architecture. Each wrapper now carries per-operation awaitable_ops tables so that heterogeneous async return types (io_result<>, io_result<size_t>, io_result<span<const_buffer>>) share a single pre-allocated cache slot with zero steady-state allocation. Introduce compile-time concept forwarding: when the wrapped type satisfies a richer concept than the wrapper's minimum (e.g. WriteSink on any_buffer_sink, ReadSource on any_buffer_source), the vtable is populated at compile time via consteval make_vtable() using if-constexpr branches and requires-clauses. At runtime a single null-pointer check selects the native forwarding path, avoiding the synthesised prepare/commit or pull/consume round-trip and its extra buffer_copy. Reorder data members for cache-line coherence: hot-path pointers (source/sink, vtable, cached awaitable, active ops) pack into the first 32 bytes; cold members (storage, secondary ops) trail within the same 64-byte line. Unify buffer sequence windowing through buffer_param across all read/write paths; add buffer_param::more() and union-based storage to eliminate default buffer descriptor initialisation. Extend buffer_array with iterator-range constructors (truncating and throwing). Rewrite the ReadStream design document from first principles, reconstructing the errors-exclude-data contract from POSIX and Win32 system-call semantics, Asio source-code analysis, and composed-operation requirements. Add three new design documents: BufferSink, BufferSource (with native-forwarding dispatch description), Run API Design, and Type-Erasing Awaitables (flat vs split vtable, cache-line analysis, construction-point trade-offs). Work around a GCC 14 ICE in expand_expr_real_1 triggered by abbreviated function template syntax inside coroutine bodies (write_now::operator()). Add ~2,300 lines of new unit tests covering every wrapper, mock, concept check, and forwarding path, bringing the suite to 74,383 assertions across 65 suites with zero failures. 47 files changed, 7589 insertions, 905 deletions.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
example/stream-pipeline/stream_pipeline.cpp (1)
20-22:⚠️ Potential issue | 🟡 MinorStale file overview: "task<>" should be "io_task<>".
Line 21 still references
task<>coroutines, butpull()now returnsio_task<std::span<const_buffer>>. Update for consistency with the code.-// The transforms use task<> coroutines for their pull() methods, allowing +// The transforms use io_task<> coroutines for their pull() methods, allowinginclude/boost/capy/io/any_buffer_source.hpp (2)
551-561:⚠️ Potential issue | 🟠 MajorMissing active awaitable cleanup in destructor — potential resource leak on cancellation.
If the
any_buffer_sourceis destroyed while an operation is in-flight (e.g., the owning coroutine is cancelled mid-pullor mid-read_some_), the awaitable sitting incached_awaitable_is freed without running its destructor. The sibling classany_read_source(lines 402–407) handles this correctly by checkingactive_ops_before freeing.The same concern applies to
active_read_ops_.Proposed fix
inline any_buffer_source::~any_buffer_source() { if(storage_) { vt_->destroy(source_); ::operator delete(storage_); } if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_read_ops_) + active_read_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + } }
563-583:⚠️ Potential issue | 🟠 MajorSame missing cleanup in move assignment operator.
Consistent with the destructor issue above,
operator=freescached_awaitable_without destroying any in-flight awaitable. Theany_read_sourcecounterpart (lines 420–425) does this correctly.Proposed fix
if(cached_awaitable_) + { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + else if(active_read_ops_) + active_read_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }include/boost/capy/io/any_buffer_sink.hpp (2)
725-734:⚠️ Potential issue | 🔴 CriticalDestructor leaks active awaitables — missing cleanup before freeing
cached_awaitable_.When an
any_buffer_sinkis destroyed while acommit,commit_eof, or write-forwarding operation is in flight (e.g., the owning coroutine is destroyed mid-suspension), the awaitable object placed incached_awaitable_is never destructed. Its memory is freed but its destructor is skipped.Compare with
any_write_sink::~any_write_sink()(Lines 526–541 inany_write_sink.hpp) which correctly checksactive_write_ops_/active_eof_ops_and callsdestroybefore::operator delete.Proposed fix
inline any_buffer_sink::~any_buffer_sink() { if(storage_) { vt_->destroy(sink_); ::operator delete(storage_); } if(cached_awaitable_) + { + if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); + else if(active_ops_) + active_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + } }
737-757:⚠️ Potential issue | 🔴 CriticalMove-assignment has the same missing awaitable destruction.
Same issue as the destructor:
cached_awaitable_is freed without destroying any active awaitable object living inside it.Proposed fix
if(cached_awaitable_) + { + if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); + else if(active_ops_) + active_ops_->destroy(cached_awaitable_); ::operator delete(cached_awaitable_); + }
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/write_now.hpp`:
- Around line 354-356: The comment incorrectly attributes the co_yield split to
a "GCC ICE" and "GCC path" while placed in the non-GCC (`#else`) branch; update
the comment near the consuming_buffers/co_yield code (the block referencing
expand_expr_real_1 and co_yield) to clearly state that the non-GCC path
intentionally uses co_yield to separate the eager synchronous loop from the
fallback coroutine path (e.g., "The non-GCC path uses co_yield to split the
eager synchronous loop from the fallback coroutine path, preventing ..."), and
remove or rephrase the GCC-specific wording so it accurately documents why this
branch uses the split.
🧹 Nitpick comments (4)
include/boost/capy/io/write_now.hpp (2)
291-324: Operator() javadoc is missing a few guideline-required sections.Per the coding guidelines for async/awaitable functions, the following sections are absent:
- Cancellation support — state whether the operation supports cancellation via
stop_tokenor the stream'scancel(), and that the resulting error compares equal tocond::canceled.- @throws — document precondition violations (e.g., calling while a previous op is outstanding) or explicitly state "no exceptions during normal operation."
- Concurrency / overlap — state that only one outstanding call is permitted (partially covered in the class-level doc, but the per-function guideline asks for it here too).
These are all called out explicitly in the coding guidelines for async/awaitable function javadoc. As per coding guidelines: "Document cancellation support in async/awaitable function javadoc", "Document
@throwsin async/awaitable function javadoc for precondition violations", and "Document concurrency and overlap in async/awaitable function javadoc".
108-115: GCC workaround:suspend_alwaysdisables the entire eager fast-path.Under the GCC workaround,
initial_suspendreturnssuspend_always, so the coroutine never runs eagerly. This means every call goes through the async path regardless of whetherwrite_somecould have completed synchronously. That's presumably intentional given the ICE, but worth a brief comment in the#ifblock noting the performance trade-off for readers unfamiliar with the workaround.include/boost/capy/io/any_read_source.hpp (1)
575-596: Inconsistent CTAD usage:buffer_param bp(buffers)vs.buffer_param<MB> bp(buffers).Line 579 uses
buffer_param bp(buffers)(relying on CTAD), whileany_buffer_source::readandany_buffer_source::read_someuse the explicit formbuffer_param<MB> bp(buffers). Both should work if a deduction guide exists, but it's a minor inconsistency across the two files.include/boost/capy/io/any_buffer_sink.hpp (1)
643-689:compute_max_size/compute_max_alignre-derive awaitable types already aliased in the WriteSink-specific functions.Each
construct_write_*_awaitable_implfunction locally defines ausing Aw = ...that is the same type beingsizeof'd here. Consider adding top-level type aliases (likeWriteSomeAwaitable, etc.) gated byWriteSink<S>— asany_write_sink::vtable_for_implalready does — to avoid the duplication and the risk of divergence.
| // GCC ICE in expand_expr_real_1 (expr.cc:11376) | ||
| // when consuming_buffers spans a co_yield, so | ||
| // the GCC path uses a separate simple coroutine. |
There was a problem hiding this comment.
Comment references GCC ICE but this is the non-GCC path.
The comment on lines 354–356 says "GCC ICE … so the GCC path uses a separate simple coroutine", but this block is the #else branch (i.e., the non-GCC path). The comment should explain why the non-GCC path uses the co_yield split instead — e.g., "The non-GCC path uses co_yield to split the eager synchronous loop from the fallback coroutine path." The current wording is confusing to a reader.
Suggested comment fix
- // GCC ICE in expand_expr_real_1 (expr.cc:11376)
- // when consuming_buffers spans a co_yield, so
- // the GCC path uses a separate simple coroutine.
+ // Try to complete synchronously; if any write_some
+ // needs to suspend, fall through to the coroutine
+ // slow path after co_yield. The GCC build uses a
+ // separate branch (`#if` above) because GCC ICEs when
+ // consuming_buffers spans a co_yield.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GCC ICE in expand_expr_real_1 (expr.cc:11376) | |
| // when consuming_buffers spans a co_yield, so | |
| // the GCC path uses a separate simple coroutine. | |
| // Try to complete synchronously; if any write_some | |
| // needs to suspend, fall through to the coroutine | |
| // slow path after co_yield. The GCC build uses a | |
| // separate branch (`#if` above) because GCC ICEs when | |
| // consuming_buffers spans a co_yield. |
🤖 Prompt for AI Agents
In `@include/boost/capy/io/write_now.hpp` around lines 354 - 356, The comment
incorrectly attributes the co_yield split to a "GCC ICE" and "GCC path" while
placed in the non-GCC (`#else`) branch; update the comment near the
consuming_buffers/co_yield code (the block referencing expand_expr_real_1 and
co_yield) to clearly state that the non-GCC path intentionally uses co_yield to
separate the eager synchronous loop from the fallback coroutine path (e.g., "The
non-GCC path uses co_yield to split the eager synchronous loop from the fallback
coroutine path, preventing ..."), and remove or rephrase the GCC-specific
wording so it accurately documents why this branch uses the split.
269838b to
4a9c6b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/write_now.hpp`:
- Around line 348-358: The loop in write_now (the block that calls
stream_.write_some) can spin if write_some returns zero bytes with no error; add
a guard after the call to stream_.write_some to detect r.t1 == 0 and treat it as
a failure: construct and return an appropriate io_result<std::size_t> (include
an error code or make a specific error like EOF/connection_reset) with
total_written, so the coroutine exits instead of looping; apply the same
zero-byte check/return in the corresponding non-GCC code path as well to keep
behavior consistent.
🧹 Nitpick comments (2)
include/boost/capy/io/write_now.hpp (2)
374-385: Discarded awaitable whenawait_ready()returns false.On line 376, if
inner.await_ready()is false,inneris destroyed withoutawait_suspendever being called. This is safe only if the stream'swrite_someawaitable doesn't start any asynchronous work untilawait_suspend. If this is an established invariant ofIoAwaitablein this library, a brief comment here would help readers understand why the early break is safe.
307-340: Async function javadoc is incomplete per coding guidelines.The
operator()documentation is a good start but is missing several sections required by the coding guidelines for async/awaitable functions:
- Completion conditions as
@lilist (e.g., "The entire buffer sequence is written", "An error occurs", "The operation was canceled")- Concurrency/overlap section (only one outstanding operation at a time — already in class doc but should be restated)
@throwsdocumentation (precondition violations or "no exceptions during normal operation")- Cancellation support details (stop_token, cancel() mechanism)
- Multiple
@par Exampleblocks (guidelines call for 2–3 showing happy path, error handling, cancellation)@note/@par Remarksfor behavioral gotchas (e.g., frame caching, GCC workaround)As per coding guidelines: "Document completion conditions in async/awaitable function javadoc using bulleted
@lilist", "Document concurrency and overlap in async/awaitable function javadoc", "Include@throwsin async/awaitable function javadoc", "Document cancellation support in async/awaitable function javadoc", and "Include@parExample in async/awaitable function javadoc with two or three@codeblocks showing different usage patterns".
include/boost/capy/io/write_now.hpp
Outdated
| while(total_written < total_size) | ||
| { | ||
| auto r = co_await stream_.write_some( | ||
| detail::make_consumed(buffers, total_written)); | ||
| if(r.ec) | ||
| { | ||
| io_result<std::size_t> result{r.ec, total_written}; | ||
| co_return result; | ||
| } | ||
| total_written += r.t1; | ||
| } |
There was a problem hiding this comment.
Potential infinite loop if write_some returns zero bytes without error.
If stream_.write_some(...) returns {no_error, 0} (e.g., a zero-length buffer or a misbehaving stream), total_written never advances and this loop spins forever. The same applies to the non-GCC path. Consider guarding against a zero-byte return, or documenting that WriteStream::write_some must return at least one byte on success.
🤖 Prompt for AI Agents
In `@include/boost/capy/io/write_now.hpp` around lines 348 - 358, The loop in
write_now (the block that calls stream_.write_some) can spin if write_some
returns zero bytes with no error; add a guard after the call to
stream_.write_some to detect r.t1 == 0 and treat it as a failure: construct and
return an appropriate io_result<std::size_t> (include an error code or make a
specific error like EOF/connection_reset) with total_written, so the coroutine
exits instead of looping; apply the same zero-byte check/return in the
corresponding non-GCC code path as well to keep behavior consistent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/write_now.hpp`:
- Around line 341-349: The GCC workaround branch controlled by
BOOST_CAPY_WRITE_NOW_WORKAROUND implements operator()(Buffers buffers) in
op_type as a placeholder that always returns io_result<std::size_t>{{}, 0},
effectively dropping all data; replace this stub with a real implementation that
mirrors the non-workaround write_now behavior: iterate over the provided Buffers
(template ConstBufferSequence) and perform the same synchronous/asynchronous
write logic used by the normal path, accumulate bytes written into the
io_result<std::size_t>, and propagate any errors instead of returning success;
locate the operator() overload in the op_type class and the write_now call sites
to copy or refactor the actual write semantics into the workaround branch so GCC
builds behave identically to other compilers.
- Around line 364-366: Document in the WriteStream concept that any awaitable
returned by WriteStream::write_some must be safe to destroy without calling
await_suspend (i.e., its constructor/await_ready must not enqueue work, acquire
resources needing cleanup in await_suspend, or store state that requires
await_suspend to run). Update the concept/README or API doc near
WriteStream::write_some and the write_now.hpp comments to state this contract
explicitly and mention inner.await_ready() use case and await_suspend
requirements so implementations (e.g., test::write_stream) do not rely on
await_suspend running for cleanup.
🧹 Nitpick comments (4)
example/type-erased-echo/main.cpp (1)
13-13: Unused include:boost/capy/test/fuse.hpp
test::fuseis not used anywhere in this file. Consider removing this include.Proposed fix
-#include <boost/capy/test/fuse.hpp>example/mock-stream-testing/mock_stream_testing.cpp (1)
23-23:echo_line_uppercaselacks a brief docstring.This is a non-trivial protocol function in an example file that serves as documentation itself. A brief Javadoc comment describing the protocol (reads until newline, echoes back uppercase) would help readers, especially since the coding guidelines note that docstrings should be present for all functions. The inline comment on Line 21 partially serves this purpose, but a
/** ... */block before the signature would be more discoverable.As per coding guidelines: "Docstrings are required for all classes and functions in public headers in non-detail namespaces." While this is an example file rather than a public header, adding a brief doc would improve its value as reference material.
include/boost/capy/io/write_now.hpp (2)
307-340: Async function javadoc is missing several sections required by coding guidelines.The documentation covers the basics well, but per the coding guidelines for async/awaitable functions, the following sections are missing:
- Completion conditions as a bulleted
@lilist (e.g., "The supplied buffers are fully written", "An error occurs", "The operation was canceled")- Concurrency/overlap — state that only one operation may be outstanding (this is on the class doc but should be reiterated or referenced)
@throws— document precondition violations or state "no exceptions during normal operation"- Cancellation support — document how cancellation works (stop_token) and that the resulting error compares equal to
capy::cond::canceled@note/@par Remarks— document the eager fast-path behavior as a behavioral gotcha/equivalenceAs per coding guidelines: async/awaitable function javadoc should document completion conditions, concurrency,
@throws, cancellation support, and behavioral notes.
40-54: Remove unuseddetail::make_consumedhelper or clarify its intended use.The
make_consumedfunction at lines 40–54 is defined but never called anywhere in the codebase, including the GCC workaround path it was documented to support (which is currently a stub). Either remove it if it is no longer needed, or retain it with a clearer comment explaining future intent if it is reserved for planned implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/boost/capy/io/any_buffer_sink.hpp (3)
741-751:⚠️ Potential issue | 🟠 MajorActive awaitable is not destroyed if the coroutine is cancelled while suspended.
If a coroutine is destroyed while suspended on one of these awaitables (e.g., due to cancellation or scope exit), the inner awaitable placement-new'd into
cached_awaitable_is never destroyed —await_resume's guard is the only cleanup path. The destructor here frees the raw memory but skips the awaitable's destructor.Consider destroying the active awaitable in the destructor:
🛡️ Proposed fix
any_buffer_sink::~any_buffer_sink() { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); if(storage_) { vt_->destroy(sink_); ::operator delete(storage_); } if(cached_awaitable_) ::operator delete(cached_awaitable_); }The same concern applies to the move-assignment operator (lines 753–773), which should also destroy any active awaitable before transferring ownership.
753-773:⚠️ Potential issue | 🟠 MajorMove-assignment doesn't destroy active awaitables either.
Same issue as the destructor — if
active_ops_oractive_write_ops_is non-null when move-assigned, the inner awaitable leaks.🛡️ Proposed fix
any_buffer_sink& any_buffer_sink::operator=(any_buffer_sink&& other) noexcept { if(this != &other) { + if(active_ops_) + active_ops_->destroy(cached_awaitable_); + if(active_write_ops_) + active_write_ops_->destroy(cached_awaitable_); if(storage_) { vt_->destroy(sink_);
793-797:⚠️ Potential issue | 🟡 MinorUse aligned allocation for
cached_awaitable_based on computed alignment requirement.
compute_max_align()computes the maximum alignment across all awaitable types, which is stored invt_->awaitable_alignbut never used during allocation. The plain::operator new(size)overload only guarantees__STDCPP_DEFAULT_NEW_ALIGNMENT__. Both allocation sites (lines 796 and 806) must use the aligned form:cached_awaitable_ = ::operator new(vt_->awaitable_size, std::align_val_t{vt_->awaitable_align});The corresponding
::operator deletecalls in the destructor (line 750) and move-assignment operator (line 764) must also include the alignment parameter:::operator delete(cached_awaitable_, std::align_val_t{vt_->awaitable_align});
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/any_buffer_sink.hpp`:
- Around line 1198-1231: In any_buffer_sink::write_eof change the native-path
parameter type from const_buffer_param<CB> to buffer_param<CB> so both the
native (where currently const_buffer_param<CB> bp(buffers)) and synthesized
paths use the same buffer_param<CB> helper; update the local variable name if
needed to match usage (bp.consume, bp.data, bp.more) and follow the same style
used by read_some()/write_some() elsewhere in the class.
🧹 Nitpick comments (6)
include/boost/capy/io/write_now.hpp (3)
307-340:operator()javadoc is missing several sections required by the async/awaitable function guidelines.Per the coding guidelines for async/awaitable function javadoc, the following are absent:
- Completion conditions (
@lilist): e.g., "The entire buffer sequence is written", "An error occurs", "The operation was canceled".- Concurrency / overlap: state that only one
operator()call may be in flight at a time (partially covered in the class doc, but should also appear here).- Cancellation support: document whether stop_token /
cancel()is supported and that the resulting error compares equal tocapy::cond::canceled.@throws: state precondition violations or "No exceptions are thrown during normal operation."@tparam Buffers: document the concept requirement (ConstBufferSequence).@note/@par Remarks: mention that the synchronous fast-path avoids coroutine suspension entirely.The
@returnformat correctly uses(error_code,std::size_t)— nice.As per coding guidelines: "Document completion conditions in async/awaitable function javadoc using bulleted
@lilist", "Document cancellation support in async/awaitable function javadoc", "Document@throwsin async/awaitable function javadoc for precondition violations", "Document@tparamin async/awaitable function javadoc for non-variadic template parameters".
380-380: Consider adding a brief comment explaining theco_yield 0purpose.The
co_yield 0serves as a suspension point to transition from the synchronous fast-path to the async slow-path, and the yielded value is discarded. A one-line//comment would help a future reader understand this isn't yielding meaningful data.Suggested clarification
+ // Suspend here; if resumed, continue with async writes. co_yield 0;
40-54: Remove unuseddetail::make_consumedhelper or document its purpose.This helper is defined but never called within
write_now.hppor anywhere else in the codebase. The associated comment indicates it was intended as a GCC ICE workaround, but if that workaround path is no longer active, remove the function. If the workaround may be needed in the future, add a comment explaining when/why it would be called and keep it; alternatively, move it to a shared utility header if other translation units might benefit.include/boost/capy/io/any_buffer_sink.hpp (3)
1100-1113: Nativewrite()path loops but delegates towrite_()which should already write all data.If
write_()forwards to the underlyingS::write(buffers)(which per theWriteSinkconcept should write all bytes), the loop andbp.consume(n)are redundant — one call should consume the full window. If the underlyingwritecan return partial results, the code is correct but the method name is misleading. If it always writes everything, the loop is dead code.This is a minor clarity concern — consider adding a brief comment explaining why the loop is needed (e.g., windowed iteration over
buffer_param).
278-354: Javadoc for the new async methods is incomplete per coding guidelines.The new
write_some,write,write_eof(buffers), andwrite_eof()javadocs are missing several sections required by the coding guidelines for async/awaitable functions:@paramwith lifetime/validity semantics, error conditions (notableerror_codevalues), cancellation support documentation,@throwsfor precondition violations,@par Examplewith usage patterns, and@seecross-references. As per coding guidelines, async/awaitable function javadoc should include all of these sections.
659-705:compute_max_sizeandcompute_max_alignduplicate type aliases.The four
usingdeclarations (WS,W,WEB,WE) are repeated identically in both functions. Consider extracting them as type aliases in thevtable_for_implstruct (conditionally, insideif constexpr-gated helpers or via a traits sub-struct) to reduce repetition.
| template<ConstBufferSequence CB> | ||
| io_task<std::size_t> | ||
| any_buffer_sink::write_eof(CB buffers) | ||
| { | ||
| // Native WriteSink path | ||
| if(vt_->construct_write_eof_buffers_awaitable) | ||
| { | ||
| const_buffer_param<CB> bp(buffers); | ||
| std::size_t total = 0; | ||
|
|
||
| for(;;) | ||
| { | ||
| auto bufs = bp.data(); | ||
| if(bufs.empty()) | ||
| { | ||
| auto [ec] = co_await write_eof(); | ||
| co_return {ec, total}; | ||
| } | ||
|
|
||
| if(!bp.more()) | ||
| { | ||
| // Last window: send atomically with EOF | ||
| auto [ec, n] = co_await write_eof_buffers_(bufs); | ||
| total += n; | ||
| co_return {ec, total}; | ||
| } | ||
|
|
||
| auto [ec, n] = co_await write_(bufs); | ||
| total += n; | ||
| if(ec) | ||
| co_return {ec, total}; | ||
| bp.consume(n); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for declarations and usage of buffer_param and const_buffer_param
rg -n 'class.*buffer_param|struct.*buffer_param' --type=hpp --type=cpp -A 5Repository: cppalliance/capy
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Find the synthesized path at line 1234 mentioned in the review
sed -n '1230,1250p' include/boost/capy/io/any_buffer_sink.hppRepository: cppalliance/capy
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Search for all usages of buffer_param and const_buffer_param in the file
rg 'const_buffer_param|buffer_param' include/boost/capy/io/any_buffer_sink.hpp -nRepository: cppalliance/capy
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Find and examine buffer_param.hpp
find . -name "buffer_param.hpp" -type fRepository: cppalliance/capy
Length of output: 105
🏁 Script executed:
#!/bin/bash
# Look for const_buffer_param definition
rg -n 'class const_buffer_param|struct const_buffer_param' --type-add 'cpp:*.{hpp,cpp,h}' -A 10Repository: cppalliance/capy
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Check the full context around line 1205 to understand the native write_eof path
sed -n '1198,1245p' include/boost/capy/io/any_buffer_sink.hppRepository: cppalliance/capy
Length of output: 1284
🏁 Script executed:
#!/bin/bash
# Examine the buffer_param.hpp file to find both const_buffer_param and buffer_param definitions
cat -n include/boost/capy/buffers/buffer_param.hpp | head -100Repository: cppalliance/capy
Length of output: 4087
🏁 Script executed:
#!/bin/bash
# Search for const_buffer_param more broadly (it might be a template alias)
rg 'const_buffer_param' include/boost/capy/buffers/buffer_param.hpp -B 2 -A 2Repository: cppalliance/capy
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Read the full buffer_param class definition to understand the second template parameter
sed -n '100,200p' include/boost/capy/buffers/buffer_param.hppRepository: cppalliance/capy
Length of output: 2667
🏁 Script executed:
#!/bin/bash
# Continue reading the class definition
sed -n '200,300p' include/boost/capy/buffers/buffer_param.hppRepository: cppalliance/capy
Length of output: 1410
🏁 Script executed:
#!/bin/bash
# Get line count to know the full extent
wc -l include/boost/capy/buffers/buffer_param.hppRepository: cppalliance/capy
Length of output: 107
Unify buffer_param vs const_buffer_param usage in write_eof() for consistency.
Line 1205 uses const_buffer_param<CB> in the native path while line 1234 uses buffer_param<CB> in the synthesized path. Both reference ConstBufferSequence and produce identical const_buffer storage via the buffer_type member. The inconsistency is misleading: const_buffer_param is merely buffer_param<BS, true>, so the explicit vs. implicit specification adds no semantic difference here. Use buffer_param<CB> in both paths to match the style of read_some() and write_some() elsewhere in the class.
🤖 Prompt for AI Agents
In `@include/boost/capy/io/any_buffer_sink.hpp` around lines 1198 - 1231, In
any_buffer_sink::write_eof change the native-path parameter type from
const_buffer_param<CB> to buffer_param<CB> so both the native (where currently
const_buffer_param<CB> bp(buffers)) and synthesized paths use the same
buffer_param<CB> helper; update the local variable name if needed to match usage
(bp.consume, bp.data, bp.more) and follow the same style used by
read_some()/write_some() elsewhere in the class.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/write_now.hpp`:
- Around line 119-148: The erased_io_awaiter currently leaks heap memory when a
coroutine is destroyed while suspended because inner_ is only freed in
await_resume(); add a destructor to erased_io_awaiter that checks if inner_ is
non-null and calls destroy_(inner_) (then set inner_ = nullptr) so the allocated
awaitable is freed on destruction; ensure this destructor coexists with the
comment about "trivially destructible aggregate" (if adding a destructor
re-triggers the GCC ICE, instead add equivalent cleanup in promise_type's
destructor or use a small RAII guard wrapper created where inner_ is new'd that
will call destroy_ on scope exit), and keep await_resume()'s existing
destroy_/nulling so normal completion remains no-op for the destructor.
🧹 Nitpick comments (4)
include/boost/capy/io/write_now.hpp (4)
271-286: Frame cache reuse doesn't account for size mismatch across differentBufferstypes.If
operator()is called with different buffer types that produce different coroutine frame sizes, and the first call's frame is larger, the cache is reused (line 278:cached_size_ >= size). If the second call's frame is larger, the old frame is freed and a new one allocated (lines 280-284). This logic is correct.However, the cached frame memory is live — it still holds the coroutine state from the previous (completed) call. The coroutine handle was destroyed in
~op_type()(line 299), but the memory wasn't freed (no-opoperator deleteon line 289). On the next call,operator newreturns the same pointer, and the coroutine machinery constructs a new frame in-place. This is the intended design and works correctly becauseop_type's destructor destroys the coroutine (releasing sub-objects) before the nextoperator()call.Worth a brief inline comment clarifying this invariant for future maintainers.
366-399: Async function javadoc is missing several required elements per coding guidelines.The operator's javadoc covers the brief, extended description,
@param,@return, example, and@see, which is good. However, per the coding guidelines for async/awaitable functions, the following are missing:
- Completion conditions as a bulleted
@lilist (e.g., "The entire buffer sequence has been written", "An error occurs", "The operation was canceled")- Concurrency/overlap section stating that only one operation may be outstanding (partially covered in the class-level doc but should be reiterated here)
- Cancellation support — whether it supports
stop_tokenor the stream'scancel(), and that the error compares equal tocond::canceled@throws— whether precondition violations throw or no exceptions are thrown during normal operation@note— behavioral gotchas (e.g., the eager sync fast-path behavior, frame reuse semantics)As per coding guidelines: async/awaitable function javadoc should include completion conditions, concurrency, cancellation,
@throws, and@notesections.
158-165: Workaround path starts suspended; non-workaround starts eagerly — intentional asymmetry.On the workaround path,
initial_suspendreturnssuspend_always, meaning the coroutine doesn't begin untilawait_suspendresumes it. On the non-workaround path, it returnssuspend_never, allowing the eager sync loop to run immediately during construction. This is the core of the eager-write design. Theop_type::await_ready()(line 314) checksdone_to detect if the eager path completed everything.For the workaround path: since it always suspends initially, the entire operation is never eager —
await_ready()will always returnfalse. This means the workaround path forgoes the synchronous fast-path optimization entirely. If this is intentional (simplicity to avoid the ICE), a brief comment would help clarify.
97-103:write_nowis non-movable — consider documenting this or adding deleted move operations.Copy is explicitly deleted (lines 363-364), but move constructor/assignment are implicitly deleted (due to deleted copy). Since this is a type users construct and hold, explicitly deleting or documenting the non-movable constraint avoids confusion.
Suggested addition
write_now(write_now const&) = delete; write_now& operator=(write_now const&) = delete; + write_now(write_now&&) = delete; + write_now& operator=(write_now&&) = delete;
| struct erased_io_awaiter | ||
| { | ||
| void* inner_; | ||
| promise_type* p_; | ||
| bool (*ready_)(void*); | ||
| coro (*suspend_)( | ||
| void*, coro, | ||
| executor_ref, std::stop_token); | ||
| io_result<std::size_t> (*resume_)(void*); | ||
| void (*destroy_)(void*); | ||
|
|
||
| bool await_ready() | ||
| { | ||
| return ready_(inner_); | ||
| } | ||
|
|
||
| coro await_suspend(coro h) | ||
| { | ||
| return suspend_( | ||
| inner_, h, | ||
| p_->ex_, p_->token_); | ||
| } | ||
|
|
||
| io_result<std::size_t> await_resume() | ||
| { | ||
| auto r = resume_(inner_); | ||
| destroy_(inner_); | ||
| inner_ = nullptr; | ||
| return r; | ||
| } |
There was a problem hiding this comment.
erased_io_awaiter leaks the heap-allocated awaitable if the coroutine is destroyed while suspended.
inner_ is allocated with new (line 217) but destroy_ is only called inside await_resume() (line 145). If the coroutine is destroyed while suspended on this awaiter (e.g., cancellation, parent scope teardown), the trivially-destructible erased_io_awaiter won't free inner_, leaking the heap object.
Consider adding a destructor that calls destroy_ when inner_ is non-null:
Proposed fix
struct erased_io_awaiter
{
void* inner_;
promise_type* p_;
bool (*ready_)(void*);
coro (*suspend_)(
void*, coro,
executor_ref, std::stop_token);
io_result<std::size_t> (*resume_)(void*);
void (*destroy_)(void*);
+ ~erased_io_awaiter()
+ {
+ if(inner_)
+ destroy_(inner_);
+ }
+
+ erased_io_awaiter(erased_io_awaiter const&) = delete;
+ erased_io_awaiter& operator=(erased_io_awaiter const&) = delete;
+ erased_io_awaiter(erased_io_awaiter&& other) noexcept
+ : inner_(std::exchange(other.inner_, nullptr))
+ , p_(other.p_)
+ , ready_(other.ready_)
+ , suspend_(other.suspend_)
+ , resume_(other.resume_)
+ , destroy_(other.destroy_)
+ {
+ }
+
bool await_ready()
{
return ready_(inner_);
}Then in await_resume, the null-out on line 146 ensures the destructor becomes a no-op after normal completion.
Note: The comment on line 117 says "trivially destructible aggregate" is needed for the GCC ICE bisection. If adding a destructor re-triggers the ICE, an alternative is a guard wrapper or explicit cleanup in promise_type's destructor.
🤖 Prompt for AI Agents
In `@include/boost/capy/io/write_now.hpp` around lines 119 - 148, The
erased_io_awaiter currently leaks heap memory when a coroutine is destroyed
while suspended because inner_ is only freed in await_resume(); add a destructor
to erased_io_awaiter that checks if inner_ is non-null and calls
destroy_(inner_) (then set inner_ = nullptr) so the allocated awaitable is freed
on destruction; ensure this destructor coexists with the comment about
"trivially destructible aggregate" (if adding a destructor re-triggers the GCC
ICE, instead add equivalent cleanup in promise_type's destructor or use a small
RAII guard wrapper created where inner_ is new'd that will call destroy_ on
scope exit), and keep await_resume()'s existing destroy_/nulling so normal
completion remains no-op for the destructor.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@include/boost/capy/io/write_now.hpp`:
- Around line 368-377: The GCC workaround path is currently discarding the real
write_some awaitable via await_transform and using erased_io_awaiter, so
operator()(Buffers buffers) never performs I/O and returns a fake buffer_size;
change the implementation so the real stream_.write_some awaitable is awaited
and its result propagated: either adjust await_transform/erased_io_awaiter to
forward the original write_some awaitable (preserving its resume/return
behavior) or modify operator()(Buffers buffers) to directly co_await
stream_.write_some(buffers) and build the io_result from that actual result (use
the returned bytes transferred rather than buffer_size(buffers)); ensure symbols
touched include await_transform, erased_io_awaiter, operator()(Buffers),
stream_.write_some and buffer_size so the real I/O occurs and the true result is
returned.
- Around line 40-52: The helper function detail::make_consumed (templated on
ConstBufferSequence, returning consuming_buffers<Buffers> and calling
cb.consume(n)) is unused and should be removed to eliminate dead code; delete
its full definition from include/boost/capy/io/write_now.hpp (and any
corresponding forward-declaration) unless you intend it to be public—if so, move
the template to a shared header where consuming_buffers and ConstBufferSequence
are visible and update includes accordingly.
🧹 Nitpick comments (3)
include/boost/capy/io/write_now.hpp (3)
334-367:operator()javadoc is missing several guideline-required sections.Per the coding guidelines for async/awaitable function javadoc, the following are absent:
- Completion conditions (
@lilist — e.g., "All buffers are written", "An error occurs", "The operation was canceled")- Concurrency/overlap (state that only one operation may be in flight)
@throws(precondition violations or "no exceptions during normal operation")- Cancellation support (via stop_token or cancel(), resulting error compares equal to
capy::cond::canceled)@tparam Bufferswith concept requirement (ConstBufferSequence)The
@returndescription is good and follows the(error_code,std::size_t)format correctly.As per coding guidelines: "Document completion conditions in async/awaitable function javadoc using bulleted
@lilist", "Document concurrency and overlap", "Document@throws", "Document cancellation support", "Document@tparamfor non-variadic template parameters".
196-237:await_transformIoAwaitable branch on the GCC workaround path silently discards the awaitable.On line 204,
(void)a;suppresses the unused warning, but the awaitable — and any side effects of its construction — are silently dropped. Ifwrite_someawaitables ever acquire resources in construction (e.g., register with an I/O queue), this would leak or cause protocol errors. This is acceptable for a bisect stub, but worth a// FIXMEorstatic_assertto prevent accidental use in production.
144-151: Workaround path starts suspended, normal path starts eagerly — intentional asymmetry worth documenting.The GCC path uses
suspend_alwaysforinitial_suspendwhile the non-GCC path usessuspend_never. This means the coroutine body executes eagerly only on the non-GCC path (enabling the synchronous fast-path). A brief comment explaining why the GCC path must start suspended (beyond the bisect note) would help future maintainers.
| namespace detail { | ||
|
|
||
| // Keeps consuming_buffers out of coroutine frames (GCC ICE workaround) | ||
| template<ConstBufferSequence Buffers> | ||
| consuming_buffers<Buffers> | ||
| make_consumed( | ||
| Buffers const& buffers, | ||
| std::size_t n) noexcept | ||
| { | ||
| consuming_buffers<Buffers> cb(buffers); | ||
| cb.consume(n); | ||
| return cb; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "make_consumed" --type-add 'cpp:*.{hpp,cpp,h,cc,cxx}' --type cppRepository: cppalliance/capy
Length of output: 113
Remove unused detail::make_consumed helper function.
This function is defined at lines 45–52 but is never called anywhere in the codebase. If it was intended for use outside this translation unit, move it to a shared header; otherwise, remove it as dead code.
🤖 Prompt for AI Agents
In `@include/boost/capy/io/write_now.hpp` around lines 40 - 52, The helper
function detail::make_consumed (templated on ConstBufferSequence, returning
consuming_buffers<Buffers> and calling cb.consume(n)) is unused and should be
removed to eliminate dead code; delete its full definition from
include/boost/capy/io/write_now.hpp (and any corresponding forward-declaration)
unless you intend it to be public—if so, move the template to a shared header
where consuming_buffers and ConstBufferSequence are visible and update includes
accordingly.
| #if BOOST_CAPY_WRITE_NOW_WORKAROUND | ||
| template<ConstBufferSequence Buffers> | ||
| op_type | ||
| operator()(Buffers buffers) | ||
| { | ||
| // BISECT: co_await with custom suspend_always clone | ||
| co_await stream_.write_some(buffers); | ||
| io_result<std::size_t> result{{}, buffer_size(buffers)}; | ||
| co_return result; | ||
| } |
There was a problem hiding this comment.
GCC workaround path does not perform actual I/O.
await_transform discards the write_some awaitable (line 204) and substitutes erased_io_awaiter (a suspend-always stub), so no data reaches the stream. The coroutine then returns buffer_size(buffers) as if everything succeeded.
I understand from learnings that this PR is a work-in-progress kept open for CI bisecting. Flagging to ensure it isn't merged in this state. Based on learnings: "User vinniefalco keeps pull requests open during ongoing work to trigger CI, even when the work is not yet ready for final review."
🤖 Prompt for AI Agents
In `@include/boost/capy/io/write_now.hpp` around lines 368 - 377, The GCC
workaround path is currently discarding the real write_some awaitable via
await_transform and using erased_io_awaiter, so operator()(Buffers buffers)
never performs I/O and returns a fake buffer_size; change the implementation so
the real stream_.write_some awaitable is awaited and its result propagated:
either adjust await_transform/erased_io_awaiter to forward the original
write_some awaitable (preserving its resume/return behavior) or modify
operator()(Buffers buffers) to directly co_await stream_.write_some(buffers) and
build the io_result from that actual result (use the returned bytes transferred
rather than buffer_size(buffers)); ensure symbols touched include
await_transform, erased_io_awaiter, operator()(Buffers), stream_.write_some and
buffer_size so the real I/O occurs and the true result is returned.
2 participants
Summary by CodeRabbit
New Features
API Changes
Bug Fixes