SqliteModernCpp / sqlite_modern_cpp

The C++14 wrapper around sqlite library
MIT License
904 stars 156 forks source link

consider allowing usage of `std::span<T>` / `gsl::span<T>` for writing blobs #184

Open lakinwecker opened 5 years ago

lakinwecker commented 5 years ago

As the title says, it would be nice to have this an option for writing blobs

etam commented 5 years ago

and std::string_view for reading strings

zauguin commented 5 years ago

@lakinwecker Thanks for the request. We want to add a blob function, such that writing blobs will use ... << blob(some_vector) instead of ... << some_vector (#125, I will resurrect/merge it soon). This is based on the idea that especially when reading, >> vector looks like you are reading multiple rows instead of one column. Also this makes it easier to add additional container types without risking to add problems with other uses. Therefore, one the blob_t request is merged, we will add this there.

@etam I don't see how this would work. std::string_view can already be used for writing strings, but a string view doesn't own the memory it is pointing to (It is a view after all). So if we allow reading std::string_view, the memory is still owned by SQLite, leading to lots of easy to miss memory leaks. Why do you need this?

lakinwecker commented 5 years ago

@zauguin would we want blob to operate over a span instead of a vector? That way we can use things like std::array<> or some other container that a span can wrap? ... << blob(some_span)

Done correctly, I think this would allow ... << blob(some_vector) and ... << blob(some_array) etc. Also, would this allow streaming of the blob data so we don't have to have it all in memory at once?

etam commented 5 years ago

@etam I don't see how this would work. std::string_view can already be used for writing strings, but a string view doesn't own the memory it is pointing to (It is a view after all). So if we allow reading std::string_view, the memory is still owned by SQLite, leading to lots of easy to miss memory leaks. Why do you need this?

There might be some misunderstanding about "reading" and "writing" terms. Maybe because the issue is about writing data and I wrote in my comment about reading data. I thought it's a good place, because it's all about adding support for types that fulfill some concept (yay, c++20 terminology), not just specific std::string and std::vector.

To make things clear: I want to be able to do this:

database << "some query with ?;" << string_view;

I'm also using a gsl::span<const std::uint8_t> as a buffer view in my application and I'd like to pass it to queries, like above.

zauguin commented 5 years ago

@etam Oh, that's what I considered "writing". This is already implemented in dev.

@lakinwecker Basically yes. blob basically returns a specially tagged span, called blob_t. Currently we will not use span under-the-hood to be compatible with the existing standard library, but once std::span is available this will change. Similar for the function blob: The idea is that it can be seen as a customization point which can be implemented for arbitrary blocks of data, e.g. std::span. At some point we will be able to base this on span, but for the moment it will start with overloads for common types. Similar to optional etc we can add feature detection for std::span and the overload already, but we have to be a bit cautious here because this keeps causing problems like #191... The gsl type will probably need some custom code or a magic define to avoid depending on GSL.

Streaming is a bit more complicated, mostly because the only API (AFAICT) SQLite has for streaming is the incremental BLOB I/O system. That only handles BLOB which are already stored in the table, to it can't really be used at the binding level. We could add a separate interface to expose this though. I will think about it, but I want to get blob merged first and hopefully release the dev branch soon, given that the last release is more than two years old :see_no_evil: . If you have a great idea how to implement this or how the interface should look like, fell free to send a PR/open a separate issue, I think it would be best to keep this focused on blobs.

lakinwecker commented 5 years ago

Waiting until it's part of the standard is an understandable approach.

niansa commented 1 year ago

It's part of the standard by now :-)