This review was sent directly to me. ---------- Forwarded message --------- From: Robert A.H. Leahy <rleahy@rleahy.ca> Date: Tue, Jan 24, 2023 at 1:04 PM Subject: Aedis Review To: klemensdavidmorgenstern@gmail.com <klemensdavidmorgenstern@gmail.com> Sending directly as I'm not on the Boost mailing list. *Knowledge of Redis* No more than can be gathered by Wikipedia. *Does this library bring real benefit to C++ developers for real world use-case?* Yes. Having an idiomatic (i.e. Asio-like) way to interact with something in C++ brings real benefit. *Do you have an application for this library?* Not immediately but Redis is in use within my company so I can see one arising in the future. *Does the API match with current best practices?* I share Zach Laine's concerns about raw strings (status quo) vs. a more structured way to pass data to the API. However I'm sympathetic to arguments that: - Redis-like products may extend the set of commands and that's important to support - Redis itself may extend the set of commands and it'd be a shame to force users to wait for a library upgrade (or worse a storm of forks) to get support for new features Additionally I see the request for a more structured way to pass data to the API as a pure extension: What exists right now is a good lower layer that something higher level can be built on top of. It's not one or the other, in a perfect world both would exist. I think the API should be more generic, for example: - Why is std::tuple used rather than supporting anything which supports the tuple protocol? - std::pmr::string is used internally by aedis::request but the documentation suggests that for deserialization purposes only std::string is supported (along with only std::vector et cetera) - Why the concrete reliance on aedis::resp3::request (rather than any type which obeys some protocol)? I don't think that aedis::basic_connection::reset_stream should exist: It marries the class too heavily to TCP-like streams and doesn't add anything beyond being a convenience wrapper for several functions of the underlying stream. If anything it should be a free function. Note that the above concern is mirrored somewhat in several parts of the code where members such as is_open are used. Several opportunities to add noexcept (conditional or otherwise) should be taken. I don't see any reason why detail::connection_base::get_executor shouldn't be noexcept, for example. I feel as though read and write buffers should be customizable rather than being married to std::pmr::string (detail/connection_base.hpp). If the implementation is going to maintain its own buffers I think there should be a way to inspect them for post mortems (e.g. generating clear and context-rich error messages). *Is the documentation helpful and clear?* Yes, although some things could be made clearer. It was unclear to me at first glance what it mean to "[r]eset[] the underlying stream" (documentation for aedis::basic_connection::reset_stream), for example. *Did you try to use it?* I did not. *What is your evaluation of the implementation?* I'd rather see operator[] (possibly combined with an assert) rather than at(). I dislike the AEDIS_CHECK_OP# macros (by virtue of the fact that they're macros and, in my opinion, substantially reduce the readability of the code at the point where they're used). In detail/connection_ops.hpp:271 the operation appears to complete sucessfully because stopping "was requested from the outside." My first instinct is that in this situation the operation should end with operation_canceled. *Please explicitly state that you either accept or reject the inclusion of this library into Boost* Accept. I don't think any of the concerns I've raised are breaking and so they can be addressed later. Thanks, --Robert