On Tue, Aug 26, 2025 at 12:14 PM Mungo Gill via Boost <boost@lists.boost.org> wrote:
BACKGROUND Vinnie asked me to take a look at this library and give my feedback. This is my first Boost review. As such this review might not take into consideration accepted Boost coding practices. Although I do not have any experience of SQLite I have been working in C++ for several years, and have myself written (proprietary) C++ wrappers around C libraries for accessing other relational databases (notably Sybase). I spent approximately 4 hours in total looking at this library. This was, therefore only a quick reading. The library does warrant a more in-depth review but I wanted to get my initial feedback to the author early. COMPILATION I compiled this using GCC-13 on ARM64 Ubuntu C++17. When I attempted to compile using C++11 and C++14 there were numerous compilation errors (most notably in example/multi_index.cpp). If code is not expected to compile under older C++ versions it should be wrapped in appropriate #if...#endif statements. Out of curiosity I tried building this agains t the boost version (1.83) that ships with Ubuntu 24 and found it fails to compile under C++20 (missing pfr/core_name.hpp). I suspect this is not a huge concern as most users building this library will also build the things on which it depends. DOCUMENTATION This is probably my largest concern. The documentation itself seems extremely light. As an example of the more "advanced" features such as virtual tables need worked examples. Without some basic knowledge of sqlite3 it would be hard to follow. Some of the reference descriptions appear to be unfinished (for example vtab::cursor has the description "Cursor needs the following member"), and many detailed descriptions are just a repetition of the single-line headline. I would also like to see the detailed descriptions provide information about what purpose it serves and how it should be used rather than simply stating what can be deduced by looking at the header. In addition, many users will look at the examples for inspiration, so those examples should have detailed step-by-step comments stating what is being done and why it is being done that way.
I understand the concern. I think about it like this: there is no way one can implement a vtable without understanding sqlite3. If I were to write such a documentation, it would be 90% description of sqlite3.
INTERFACE DESIGN AND IMPLEMENTATION As a first pass the interface struck me as being quite clean. I did spot some issues (below) but none of them are major. One thing that struck me about the 'prepare' function is the choice of placeholders. `?`-based `?1`, `?2` are used for numbered placeholders, and `$`-based `$name1`, `$name2` for named placeholders. This feels slightly inconsistent, but it matches what sqlite already does, so this point can probably also be mitigated by pointing out in the docs that the syntax is driven by sqlite3 (with appropriate link). I notice that a number of classes expose the `handle` of the connection. Is there any reason for this to be part of the public interface.
Yes, so you can use it in legacy code or just use the sqlite3 API in a way that is not intended by the library. The problem with the second is that I'd need to provide a 100% implementation of sqlite for all edge cases sqlite supports. And even if I managed to that, any sqlite update could potentially add functions that my library doesn't have or that force redesigns. It is just easier to keep underlying APIs (especially when they're as well designed as sqlite's) open and instead of locking the user down.
I would be tempted to keep it private (at least for the initial release until there is a proven use case) `cstring_ref.hpp` uses `#if defined(BOOST_NO_CXX17)` as a workaroun d for the lack of a `BOOST_CXX17_CONSTEXPR` keyword. The better solution might be to add `BOOST_CXX17_CONSTEXPR` into `boost/config.hpp` alongside `BOOST_CXX14_CONSTEXPR`.
Yes, and I'll ask the boost/config maintainers after a successful review. I don't think it's a fair use of their time to change boost/config if it's not certain that it'll be used.
`allocator` has two constructors: {{{ constexpr allocator( const allocator& other ) noexcept {} template< class U > constexpr allocator( const allocator<U>& other ) noexcept {} }}} Is the first (non-templated) constructor required?
Compiler do funny things at time. IIRC MSVC did not handle the missing copy right.
Why does error_info need additonal overloads for `format` when `__GNUC__` is defined? Is there a compiler bug on old versions of the compiler?
There's an attribute to warn when the format string is invalid.
The fact that `row` and `field` hold raw pointers to the statement makes me worry about possible lifetime issues and UB if users start returning these objects from functions. There would be a slight overhead but some form of `weak_ptr`-based mechanism might be safer.
That would introduce a lot of indirection, because the pointer itself comes from sqlite. If there was some reference counting mechanism in sqlite I'd use it.
I have some other concerns (such as the fact cstring_ref even needs to exist), but I accept that the author's hands are somewhat tied by the needs of the sqlite3 C interface.
I'd actually be happy to move the cstring_ref into boost/core, but it is essential to support null-terminated strings without too many copies.
RECOMME NDATION I think this would be a useful library, and appears to be more full-featured that the alternatives listed at the foot of the documentation. My recommendation would be to Conditionally Accept on the basis that the documentation requires further work, and the examples lack user-friendly code comments. My interface/implementation concerns above are sufficiently minor to not warrant delaying progress.
Thank you for your review!