Re: [review] [sqlite]

From: Klemens Morgenstern (klemensdavidmorgenstern_at_[hidden])
Date: 2024-11-14 13:12:58

On Thu, Nov 14, 2024 at 8:19 PM Дмитрий Архипов via Boost
<boost_at_[hidden]> wrote:
>
> ср, 13 нояб. 2024 г. в 15:31, Richard Hodges via Boost <boost_at_[hidden]>:
> > The Boost formal review of the Boost SQLITE library starts *TODAY*
>
> I have a few questions
>
> 1) I do not understand the boost_sqlite vs. boost_sqlite_ext
> situation. Should I always use the second one if I want to create an
> extension? If I want to both create an extension and use SQLite from
> the same binary, should I link to both?

If you create an extension, you'll need to call into the database differently
using the indirections in sqlite3ext.h.
In order to avoid conflicts, this library puts everything you include into
an inline namespace (sqlite::ext), to avoid linker issues.

That way you get access to the host db with the same API, but the
internal calls are different,
so you'll need to link to sqlite3-ext.

You can then use a regular sqlite3 library from the same binary, but
not the same TU.
If you accidentally include boost/sqlite in both modes, but only link
one you'll get linker errors.

>
> 2) The library can deduce struct composition in C++20. Have you
> considered using Boost.PFR to also be able to do it in C++14?

I don't exactly know what you mean here. You can use pfr in C++20 for
a static_result_set (and describe for earlier standards).

struct author
{
  std::string last_name;
  std::string first_name;
};

for (auto author : conn.query<author>("select first_name, last_name
from author"));

>
> 3) Regarding custom functions.Have you considered using multiple
> arguments for the callback rather than span<value, N>? I have overall
> weird feelings about the aggregate function API. When the passed
> callable is of a class type it should have particularly named member
> functions. That gives the impression that the state that those member
> functions use is supposed to be stored in the callable object. But
> instead it should be stored in the object of the type of the first
> parameter, and the callable object itself is used as a sort of
> database-local global. As I've said this is kind of confusing. Also,
> SQLite allows overloading based on the number of arguments. The
> current interface requires having a separate callable type for each
> such overload, because the special members cannot be overloaded or be
> templates, otherwise the deduction would not work. There also doesn't
> seem to be a way to remove functions.
>

I don't entirely disagree, but this behaviour is directly inherited
from sqlite3.
That is: every overload of a function has a state like this.
And the most likely use is that you can pass state into the function

Sqlite also guarantees that final will be called whenever the function is used,
so this state can be used to share data between the invocations of step.

> I am thinking of an interface like this:
>
> template <std::size_t N = boost::dynamic_extent, class F, class Ctx =
> std::nullptr_t>
> void create_aggregate_function(F f, Ctx ctx = Ctx{})
>
> And then the callables that need the semi-global context would be
> called as f.step(ctx, value_0, ... value_n), and if they do not need
> that context, they'd be called as f.step(value_0, ... value_n).

I don't think I fully understand. What would `f`/`this` be pointing to
when `step` gets called?

>
> 4) (not a question) You use core::string_view in some APIs. Due to a
> (very unfortunate IMO) decision by the community that type is not
> public. So, instead you need to create an alias in your library and
> reference that alias in the docs. You can document the alias to have a
> compatible API to std::string_view.
>

Unfortunate, indeed.