clickhouse client move support#441
Conversation
|
|
There was a problem hiding this comment.
Please add proper unit-tests (https://github.com/ClickHouse/clickhouse-cpp/blob/master/ut/client_ut.cpp), since tests/simple/main.cpp is not built/executed on CI/CD.
Something like:
TEST(Client, MoveConstructor) {
...
}
TEST(Client, MoveAssignment) {
...
}
|
Nice, I missed that folder and looked in test only. I used clang-format google style, let me know if you would like me to use a different code format (edit: used the project clang-format except for tied functions, I think it was more readable with one member per line but lmk). |
slabko
force-pushed
the
clickhouse_client_move
branch
from
2579b9c to
0753103
Compare
They are not used anywhere, and go straight to clickhouse::Client::Impl during initialization
There was a problem hiding this comment.
Pull request overview
This PR enables move support for clickhouse::Client (previously non-movable due to rule-of-five + stored options), and adds unit tests to validate move behavior for both Client and ClientOptions.
Changes:
- Make
Clientexplicitly move-only (adds move ctor/assignment; deletes copy operations). - Remove the
Client::options_data member and rely onImpl’s internal options storage. - Add unit tests covering
Clientmove construction/assignment andClientOptionsmove behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ut/client_ut.cpp | Adds move-related unit tests for Client and ClientOptions (including an ABI-size “tripwire” under specific toolchain macros). |
| clickhouse/client.h | Declares Client move operations and deletes copy operations; removes stored options_ member. |
| clickhouse/client.cpp | Updates constructors to no longer initialize options_; defines defaulted move ctor/assignment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // movable only | ||
| Client(Client&&); | ||
| Client& operator=(Client&&); | ||
| Client(const Client&) = delete; | ||
| Client& operator=(const Client&) = delete; |
| Client::Client(Client&&) = default; | ||
| Client& Client::operator=(Client&&) = default; |
| // below (both the initialization and the field-by-field comparison) and update | ||
| // the expected size here. Guarded to a single reference toolchain so it doesn't | ||
| // break builds on other platforms/STL implementations where the size differs. | ||
| #if defined(__linux__) && defined(__x86_64__) && defined(WITH_OPENSSL) && defined(__GLIBCXX__) |
slabko
force-pushed
the
clickhouse_client_move
branch
2 times, most recently
from
06f13bc to
ed7bc17
Compare
| TEST_P(ClientCase, ClientMoveAssign) { | ||
| Client client = std::move(*client_.get()); |
| // movable only | ||
| Client(Client&&) noexcept; | ||
| Client& operator=(Client&&) noexcept; | ||
| Client(const Client&) = delete; | ||
| Client& operator=(const Client&) = delete; |
| } | ||
|
|
||
| TEST_P(ClientCase, ClientMoveConstructor) { | ||
| Client client{std::move(*client_.get())}; |
| } | ||
|
|
||
| TEST_P(ClientCase, ClientMoveAssign) { | ||
| Client client = std::move(*client_.get()); |
slabko
force-pushed
the
clickhouse_client_move
branch
from
aea7a69 to
93fa35c
Compare
As user of the library, the absence of support for move restricts design choices as it forces to use (smart or raw) ptr on Client or to make user classes non-movable too to accommodate.
While this is not dramatic, it is not ideal either. Looking at Client implementation, it has two members:
Hence I do not see large obstacle to make Client movable but happy to have second opinions from regular contributors. This is to kick start some discussions on the topic, I can test the change further on some applications if there is no opposition to the idea.