Re: [review] [sqlite]

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2024-11-13 17:18:49

On Wed, Nov 13, 2024 at 4:31 AM Richard Hodges via Boost <
boost_at_[hidden]> wrote:

> The library is authored by Klemens Morgenstern (@klemens-morgenstern in the
>  CppLang slack).
>

This review is already off to a poor start. A valid complaint from the last
review is that discussion took place off-list, on Slack. I suggested that
future announcements might remind reviewers to have discussions here
instead of elsewhere. I see no such reminder here, and instead I see you
are making it easier to contact the library submitter on Slack. At least
you didn't also provide the link to the workspace invitation page (which is
https://cpp.al/slack)

Here are the things being said in the #boost channel regarding this library
submission:

"404 not found in review email link
https://anarthal.github.io/mysql/index.html"

"is the reference doc supposed to be complete? E.g.
https://klemens.dev/sqlite/structboost_1_1sqlite_1_1connection.html is just
mostly empty, except for decorations. For what is worth, I wanted to know
how to open the DB read-only, or using URIs, and was thus looking at the
doc, and I find none."

"these docs are so complete one gets tired of completeness when reading
them"

"Should these questions be asked on the list instead of here?"

"dunno. I'm asking a simple question so far, on expectations I should have"

"Expect what you would expect from a complete Boost library."

"I was expecting at least the signatures of functions (Ctors in this
instance), at least. thus I'm suspecting a ref doc (doxygen based?)
generation issue, and was inquiring"

"Fwiw, I don't think it's inappropriate to ask about the docs like that
here because yeah, that seems kind of just like a simple configuration
error I'm guessing"

"these are good things to ask on the list and you should state your
documentation expectations on the list"

"I intend to embed the sources in my code base, instead of compiling Boost.
Will Boost.SQLite be compatible with my existing Boost 1.84, for its
dependencies (if any)?"

"darn, all the classes docs aren't there."

"explicit connection(handle_type handle, bool take_ownership = true) that
reads inscrutably at the call site. I've taken the habit of using e.g. enum
class HasOwnership : bool; inline constexpr HasOwnership cTakeOwnership{
true }; ... so the call site reads well. Bad idea? Is that something you'd
consider?"

"I think these are great topics for the mailing list"

"I confess I was for the ML as well, but in practice, for small things like
that, I prefer the interaction here..."

"regarding the enum-class-bool idiom, I guess there's .take_ownership =
true to make it readable, but C++ doesn't have that for args, does it? Only
starting with C++20 for structs, no?"

"Well I don't know, introducing a type for a single parameter seems a bit
overkill. You usually onl nee the take_ownership = false from plugin code"

"I see. In a similar vein, you've decided not to make flags type-safe? E.g.
int flags=SQLITE_OPEN_READWRITE|SQLITE_OPEN_CREATE"

"right, because the sqlite flags are good enough. i didn't want to wrap
everything and duplicate a bunch of code for little gain. the idea is more
that the library is an extension of sqlite into C++, not a 100% wrapper"

"What about opening a connection with a custom VFS? I've seen that in our
code-base. Did you get into that? Both allowing using a custom VFS, and
wrappers for VFSs similar to your vtable one?"

"nope. I thought about it but couldn't come up with an example that was
contrived. I might add it later if someone comes with a use-case"

"The designated initializer doesn't work."

"Requiring people to move all discussions to the ML is unnecessary friction
that benefit a few at the inconvenience of many."

"The point is to create an auditable  paper trail so people can see why a
library was accepted But for something like this where it's: hey, I think
you messed up a path, it's not v. pertinent to the quality of the library
being reviewed"

"I think its very pertinent. The review evaluates the author as much as the
library if not more so, since basically Boost is getting married to the
author. A consistent pattern of paying great attention to small details
such as paths is meaningful."

"docs are updated"

"The fact is that the previous review was stained by significant discussion
taking place off-list. This was discussed after the fact, and now we are
repeating this behavior. I agree there is less friction on Slack but I also
like having robust reviews of the kind that can be referred to in the future
"

"Q&A should only be on the list if it's of value to other list members. if
a reviewer asks questions for his own education so that he can determine
whether to accept the library, these questions do not have to be on the
list, although it can be useful for them to be if other reviewers are
likely to have the same questions"

"connection::prepare has two overloads, and I supposed the 1-arg one
throws? The doc doesn't say. (it's kinda obvious, given the 3-args one, but
still). Or perhaps there's a global mention in the doc somewhere about the
throw-vs-nothrow overloads? (didn't read the manual yet)"

"interactive Q&A here is less intimidating than writing to the ML, and more
"instant", especially for small questions like mine so far."

"nothrow for those overloads is not entirely correct, because the
error-code overloads could still throw things like ENOMEM. it basically
means that the failure of the main operation is not thrown"

Should these discussions stay on Slack or should they be brought to the
list?

Thanks