czw., 4 wrz 2025 o 20:54 Klemens Morgenstern via Boost < boost@lists.boost.org> napisał(a):
On Thu, Sep 4, 2025 at 4:23 PM Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
Hi Everyone, Again, I wanted to thank Klemens for his determination in developing, improving and submitting for review the Boost.Sqlite library. I also want to thank Mohammad for managing the review.
My recommendation is to REJECT the library in its current shape, and encourage the author to resubmit the library in the future, when the issue listed below is addressed.
I am *not* well familiar with SQLite, and my review is based on studying the documentation. I haven't played with the library.
I see a great value in offering a wrapper over the native SQLite C API that offers clean, automatic, C++-style resource management. However, the way I see it, the library, in its current shape, does not offer this, or at least fails to communicate it in the documentation. This is my motivation for recommending rejection.
As a C++ programmer I have two viable options. Either accept that I am dealing with the C interface, and write a resource-managing wrapper myself, or *trust* that a library performs a clean, correct resource management.
I do not see a clear indication that class `connection` does the resource management right: 1. It is not documented 2. Constructor parameter take_ownership implies (but this is never documented) that a connection may have and use a connection handle and yet not manage its lifetime. How do I even tell if the connection object I have received does or does not manage the lifetime of the corresponding handle?
The connection can be passed to user-code from sqlite where the lifetime is managed externally. These are the only cases this option should be true.
Unless it is possible in those cases to pass a pointer to `connection` object from sqlite to user code, the next best option, as I see it, would be to have a connection_view` class, much like we have `string` and `string_view`.
I do not see a clear resource management in the class `statement`. Is
`statement` associated with a resource that needs to be managed? The class has a comment "This transfers ownership to the resultset". Ownership of what? What is the full lifetime of this implied resource? Can execute() be called multiple times? If so, do you transfer the same ownership of something to multiple places?
The overload of execute qualified with && transfers ownership of the statement to the resultset, so you can do this:
conn.prepare("select * from x where y = %1").execute(42);
where as this, does not, so you can call executor multiple times:
auto s = conn.prepare("select * from x where y = %1"); s.execute(42); s.execute(43);
This is because I introduced multiple classes (statement, resultset, row) that all use the same type (sqlite3_stmt) underneath.
I don't think replicating the C APi here with a single class is helpful, especially given the required order of operations, hence the resultset & row. Therefore I split the functionality over multiple classes but they all share a single resource.
Because a prepared-statement is the way to do parameterized queries in sqlite, there'll be a lot of uses of a statement with a single execution, hence allowing the user not to keep the statement around, but just the resultset.
I fully support introducing multiple classes (statement, resultset, row). I am still struggling with understanding what kind of resource is being managed here. From your description it looks like there is a "shared state": created in function prepare and then consumed by class resultset and released by the last of all result sets and the statement object. Is this right?
I do not see a clear resource management scheme in class `resultset`. I intuitively feel that some life-time-related thing happens each time I call read_next().
It does not.
But I do not know when and what actually happens. I also do not know in the iterator interface whether it is operator++ or operator* that corresponds to read_next().
++ like any other input_iterator.
Or if the call to begin() also involves calling read_next(). THe documentation is silent about this important aspect.
I fail to see how the example provided could be correct:
sqlite::resultset rs = conn.query("select * from users;");
do { handle_row(r.current()); } while (rs.read_next()); // read it line by line
The first call to `r.current()` is performed unguarded, without any check. How does this work given that the query can potentially return zero results?
You're right, there should be a call to `rs.done()` first if the table might be empty.
Sooo, the canonical way of iterating through the result set with the non-range interface would be: for (sqlite::resultset rs = conn.query("select * from users;"); !rs.done(); rs.read_next()) { handle_row(r.current()); } Is that right? Regards, &rzej;