Re: Boost MySQL Review

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2022-05-28 19:45:50

CC'ing the ML, as you just replied to me.

On Sun, 22 May 2022 at 14:51, Seth <bugs_at_[hidden]> 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_set<boost::string_view> fails to compile. The same
> > applies for std::unordered_set<boost::mysql::value>.
> >
> Boost support is hash support, and it's trivial to `template <> struct std::hash<mysql::value> : boost::hash<mysql::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
> > (<boost/core/detail/string_view.hpp>).
> > Could you please provide me with your comparison and any extra info I should
> > be aware of? (BTW std::unordered_set<boost::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.cpp
>
> 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 <boost/core/string_view.hpp> rather than
<boost/core/detail/string_view.hpp>? 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.