CC'ing the ML, as you just replied to me. On Sun, 22 May 2022 at 14:51, Seth wrote:

Rubén, quick responses to your questions:

...

...

TARGET_LINK_LIBRARIES(reviewtest Boost::mysql)

This shouldn't be the case at all. The second line should be sufficient. Could you please open an issue against the repo and post your CMakeLists.txt, please?

I think I understand what I did wrong/differently. I've filed the description as https://github.com/anarthal/mysql/issues/97 for potential improvement.

I think there was an error in your link_libraries command, please have a look at the issue reply. I will leave the issue open to provide an example, though.

...

Implementing MySQL semantics implies the following:

value("1") == value(1) // true value(1.0) == value(1) // true

Which reminds me of the kind of patterns that have brought us pain in PHP and JS, and would prefer to avoid. Do you think "placing values in containers" is something that adds value?

Obviously Mysql's type system is not something we want to embrace. In fact, my stance is that `mysql::value` should be for marshaling, not as a vocabulary type. As such, I do *not* think that general purpose support for "placing values in containers" adds value.

Implementation-specific specialized containers like `mysql::row` can already handle the type appropriately. In fact I note that it does more harm than good because of the... surprising (some would say, broken) semantics. Again, for library internal use those are fine, but end-users should not get a false sense of "normalcy".

Perhaps the library could document some utilities like `value_equal` or `value_hash` for explicit use.

I'm reluctant to remove operator== and operator!=. I'd say their behavior matches the one for variants and can be useful. I see your point and I'm completely up to removing the other relational operators.

...

...

3. The documentation claim "Values do not define `std::hash` yet, as `boost::string_view` does not support it" is out of date. `hash_value` has been added in 1.69.0

I can see a definition of hash_value, and from what I've read this makes it compatible with Boost containers, but the std::hash specializations are in a #ifdef 0 block. AFAIR std::hash doesn't use hash_value, and std::unordered_setboost::string_view fails to compile. The same applies for std::unordered_setboost::mysql::value.

Boost support is hash support, and it's trivial to `template <> struct std::hashmysql::value : boost::hashmysql::value` if desired.

Missed that one. Thanks. Taking your comments into account, however, I'll likely won't provide it, though.

...

...

4. On that note, consider `boost::core::string_view` for better interop ("lingua franca" string_view). If you're interested I just did an exhaustive side-by-side comparison of features and observable behaviour` which I can share.

I wasn't aware of this class. Doing a quick grep in Beast, I've found it does use it, but it seems located in a detail/ folder (). Could you please provide me with your comparison and any extra info I should be aware of? (BTW std::unordered_setboost::core::string_view doesn't compile either).

My impact analysis consists of a suite of Very Ugly(TM) test cases that have lead to differences marked "OBSERVABLE" in the test code. It's not intended for publication, and I was lazy by using C++17 features. However, it has the "code talks" quality in that it leaves little to the imagination. So, please remember to wear goggles: https://github.com/sehe/beast/blob/core_string_view/test/beast/core/string.c...

The main area I didn't compare was constexpr-ness. Anecdotally I got the impression that core::string_view is more consistent in constexpr-ification. In fact my analysis uncovered a bug in Boost Utility https://github.com/boostorg/utility/issues/94

Some other differences have been addressed for an upcoming Boost release. The related tickets can be found in this cppslack thread: https://cpplang.slack.com/archives/CD7BDP8AX/p1651467929687909

I don't think any of these differences impact this library. I've raised https://github.com/anarthal/mysql/issues/98 to address it. It still freaks me out a little including a header labeled as "detail" - why is it not available as rather than ? It looks like if the class wasn't supposed to be used in public interfaces.

...

I'd say that the pipeline interface (https://github.com/anarthal/mysql/issues/76) may be a better interface than multi-statement for this use case.

I think what I tried to describe is what you call the pipeline interface. So +1

...

Note that prepared_statement's get allocated a server-side ID, which we expose via the prepared_statement::id() function. Something that could be useful for that use case is closing a statement without a prepared_statement object, just using its ID.

It may be worth adding a hint about the native_handle with the current warning about prepared_statement's destructor.

...

Something I noticed while implementing these erased types is that having prepared_statement and resultset be I/O objects makes the erased interface much harder. As I've mentioned in other review responses, I'm thinking on replacing the following signatures:

prepared_statement::execute(const statement_params<ValueIterator>&) prepared_statement::close() resultset::read_one(row&)

By functions in the connection:

connection::execute_statement(const prepared_statement&) connection::close_statement(const prepared_statement&) connection::read_row(resultset&, row&)

Where prepared_statement and resultset would be non-templated types. What do you think of this API?

I can see how it simplifies things. It feel a bit C-api to me. But I have no strong opinion either way. It's what you think is best for users in the end.

Regards, Ruben.