Hi, here is my review. 1) ================================
https://anarthal.github.io/mysql/mysql/ref/boost__mysql__resultset/async_rea... The handler signature for this operation is void(boost::mysql::error_code, std::vectorboost::mysql::row). etc.
Having a std::vector in the signature of the completion handler is unusual in ASIO. In general, large objects should be passed as parameters to the initiating function. Additionally, this operation does not support custom allocators. That wouldn't be a big problem to me if I could reuse the same memory for each request but the interface above prevents me from doing so. It is also a pessimized storage as each row will allocate its own chunk of memory. A single block of memory would be better. 2) ================================
https://anarthal.github.io/mysql/mysql/ref/boost__mysql__connection/async_qu... The handler signature for this operation is void(boost::mysql::error_code, boost::mysql::resultset<Stream>)
Here the completion handler is returning an io object i.e. boost::mysql::resultset, which is also unusual as far as I can see. Wouldn't it make sense for users to specify a different executor for the resultset? This interface is preventing that. I prefer having the completion handler return config parameters with which I can instantiate a boost::mysql::resultset myself in my desired executor. 3) ================================
https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee...
I am not convinced the design of the value class is a good idea. The
reason is that users know at compile time what types they are
expecting from a given query. That means it would be possible for them
to specify that before doing the query. So instead of doing this
https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee...
boost::mysql::row row;
while (result.async_read_one(row, additional_info, yield[ec]))
users could do
boost::mysql::row
IMO, the examples are too limited and show variation of the same functionality. For example - Text query, async with callbacks - Text query, async with futures - Text query, async with Boost.Coroutine coroutines - Text query, async with C++20 coroutines - Default completion tokens Here you are showing nice Asio features. I am not meaning you should remove them but if I am not mistaken there is no example that shows - How to keep long lasting connections open (specially for ssl). - Timers for resolve, connect, read and write. - Synchronized writes. - Healthy checks, failover etc. I estimate that to address these points users will be required to write further 500 - 1000 LOC on top of your library. Otherwise they will be stuck on underperforming short lived connections. It would be helpful for users to have a starting point to begin with. There some minor things I have also noticed: 5) ================================
https://github.com/anarthal/mysql/blob/13d3615464f41222052d9ad8fa0c86bcdddee...
I expect this function to be O(1). 6) ================================
https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_std_optio... https://anarthal.github.io/mysql/mysql/ref/boost__mysql__value/get_optional....
Sounds also unusual to have two member functions for the different versions of optional. I suggest using the boost version until we transition to C++17. 7) ================================
https://anarthal.github.io/mysql/mysql/ref/boost__mysql__errc.html
errc should be conditions and not error AFAICS. ============= Review End ================ I vote for conditional acceptance. Points 1, 2, 3 and 4 should be addressed before acceptance. My acceptance criteria sums up to 1) Remove functionality that provides low performance. With coroutines it will be easy for users to loop on async_query to create a std::vector of rows. 2) No IO object should be returned as a parameter of the completion handler. 3) Review the value type. Can it be improved? 4) Improve examples. Regards, Marcelo