Skip to content

Conversation

ilandry
Copy link

@ilandry ilandry commented Sep 21, 2025

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:

  • a unique_ptr to Impl which by definition should have correct and well defined move
  • a const ClientInfo object, as far as I saw all members are movable with non surprising side effect (std::string, std::vector, raw ptr and native integer types), by removing the const qualifier we make sure it is moved rather than copied (which could have unintended consequence for the raw ptr especially).

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.

@CLAassistant
Copy link

CLAassistant commented Sep 21, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@Enmk Enmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
...
}

@ilandry
Copy link
Author

ilandry commented Sep 27, 2025

Nice, I missed that folder and looked in test only.
Added unit tests and removed the simple test change.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants