Repository navigation
Conversation
Did some reshuffling in Buffers docs, so that buffer counting is in buffer algrithms. Removed buffe_length from the docs as we are getting consensus to remove it altogether.
|
An automated preview of the documentation is available at https://447.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-10-10 20:04:22 UTC |
|
GCOVR code coverage report https://447.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-10-10 20:19:21 UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #447 +/- ##
========================================
Coverage 98.09% 98.09%
========================================
Files 130 130
Lines 6289 6289
========================================
Hits 6169 6169
Misses 120 120
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
mvandeberg
left a comment
There was a problem hiding this comment.
I can't accept the PR as is because it removes documentation of buffer_length without removing buffer_length. Those should both happen at the same time.
In the meantime, I am leaving my review of the doc changes itself so it can be ready to go when we remove buffer_length.
- Stale link back to 5b.
4.coroutines/4g.composition.adoc:147-148still saysread_alllives on Composite Buffers and links there. Now that it's on Buffer Algorithms, the link should bexref:5.buffers/5d.buffer-algo.adoc#_consuming_buffers[Buffer Algorithms]. The NOTE on 5d links to 4g, so the two pages should point at each other. buffer_lengthis still used elsewhere. The commit dropsbuffer_lengthfrom the buffer pages because it's slated for removal.8.examples/8c.buffer-composition.adoc:54still tells readers to usebuffer_length(), andexample/buffer-composition/buffer_composition.cppcalls it 3 times (lines 58, 93, 113). Should those change here, or in the PR that removes the function? Either way, the IMPORTANT box this PR deletes was the only place that explained the difference frombuffer_size.- Orphaned snippet tags.
test/doc/snippets/5d_buffer_algo.cppstill hastag::buffer_empty_example(line 308) andtag::buffer_length_example(line 325), but no page includes them now. The include-tag check only catches includes of missing tags, not tags nobody includes, so nothing will flag this. See the inline comment on 5d line 24 forbuffer_empty.buffer_length_example/testBufferLengthcan go now or with the function's removal. - 5b rationale now points forward.
5b.composite-buffers.adoc:155("The slice views from cpp:buffer_slice[] and cpp:consuming_buffers::data[]...") explains the bidirectional requirement using two types the reader now meets only on the later 5d page. An xref to Buffer Algorithms, or moving that reason to 5d, would fix the ordering.
|
|
||
| cpp:buffer_length[] returns the *number of buffers* in the sequence: | ||
|
|
||
| cpp:buffer_empty[] reports whether a sequence carries no data, |
There was a problem hiding this comment.
This section lost its example. The buffer_empty_example include was dropped, but the tagged, tested fragment is still in the snippet file. Could you include it again after this paragraph?
[source,cpp]
----
include::example$snippets/5d_buffer_algo.cpp[tag=buffer_empty_example,indent=0]
----
There was a problem hiding this comment.
Would it be ok, if I consequently remove the example from the cpp file.
This example contributes nothing to the user's understanding of the library, and at the same time makes the docs longer.
There was a problem hiding this comment.
The buffer_size example already contains an empty buffer. Could we add a couple of asserts in that example to handle both functions at the same time?
assert(!capy::buffer_empty(combined));
assert(capy::buffer_empty(buf3));I suggest this because buffer_empty is the only function without an example on the page. I appreciate streamlining the documentation. I think this satisfies the desire for an example without bloating the page too much.
| This would translate into later using a zero-size buffer. | ||
|
|
||
|
|
||
| == buffer_slice |
There was a problem hiding this comment.
The other headings on this page describe tasks ("Counting Bytes", "Copying", "Partial Transfer Loops"), but these two are bare identifiers (also consuming_buffers on line 52). How about == Slicing and == Consuming Buffers? The second one keeps the generated ID _consuming_buffers, so <<_consuming_buffers>> on line 108 still resolves.
There was a problem hiding this comment.
If this is ok with you, I would address it in a separate PR. A PR that rewrites the entire Buffer algorithms section.
There was a problem hiding this comment.
This is a small change the preserve formatting while the page is in an intermediate state.
| auto combined = std::array{buf1, buf2, buf3}; | ||
|
|
||
| std::size_t total = capy::buffer_size(combined); // 10 | ||
| assert(capy::buffer_size(combined) == 10); |
There was a problem hiding this comment.
Nit: the other fragments on this page show results as comments
(// 11 in buffer_copy_example, for instance). The assert does nothing under NDEBUG, and the BOOST_TEST two lines down already checks the same thing. Either style works, but one style per page would be more consistent.
There was a problem hiding this comment.
Again, would you agree to a separate PR?
There was a problem hiding this comment.
Your PR makes the change and breaks convention. This is the PR where it should be addressed.
|
@mvandeberg, thank you for the review. Could we assume the view that I am doing incremental changes to the Buffers docs, only a subsection or two at a time? |
|
@mvandeberg, rearding your list:
|
Anything that has further reaching consequences, I have rescinded from my review. The remaining items should be relevant just to this PR. I realize that some of the remaining items are nitpicks, but I am doing this because we already have people using capy and reading the documentation. It is worth the extra work of cleaning up the small messes as we remodel the documentation. |
Did some reshuffling in Buffers docs, so that buffer counting is in buffer algrithms.
Removed buffe_length from the docs as we are getting consensus to remove it altogether.