add new init method for FdTable and BufferTable#50
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds new init methods to both FdTable and BufferTable classes that allow initialization with pre-populated arrays, complementing the existing init methods that only allocate capacity. The PR also corrects documentation for the update methods to accurately reflect that they return the count of updated items rather than 0 on success.
Changes:
- Added
FdTable::init(const int *fds, unsigned nr_fds)method for initializing with file descriptor arrays - Added
BufferTable::init(const iovec *vecs, unsigned nr_vecs)method for initializing with buffer arrays - Updated documentation for
FdTable::updateandBufferTable::updateto correctly state they return the number of items updated - Added test cases for the new initialization methods
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| include/condy/ring.hpp | Added new init method overloads for FdTable and BufferTable; corrected return value documentation for update methods |
| tests/test_fd_table.cpp | Added test case for init with fd array; updated existing test to verify update return values |
| tests/test_buffer_table.cpp | Added test case for init with iovec array |
Comments suppressed due to low confidence (1)
tests/test_fd_table.cpp:75
- The test does not clean up the fd_table after initialization. Unlike the test_buffer_table.cpp which tests destroy() and multiple init() calls to verify state management (line 17-23), this test doesn't call destroy() after using init(). This could lead to resource leaks or state pollution between tests if the table is shared. Consider adding cleanup or verifying that multiple init() calls are handled correctly.
TEST_CASE("test fd_table - init with fd array") {
auto func = []() -> condy::Coro<void> {
auto &fd_table = condy::detail::Context::current().ring()->fd_table();
int pipes[2];
int ret = pipe(pipes);
REQUIRE(ret == 0);
int r = fd_table.init(pipes, 2);
REQUIRE(r == 0);
close(pipes[0]);
close(pipes[1]);
pipes[0] = -1;
pipes[1] = -1;
r = fd_table.update(0, pipes, 2);
REQUIRE(r == 2);
co_return;
};
condy::sync_wait(func());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.