std::string_view support

[dvtate]

My goal is to fix this issue: #517

Previous, similar PRs:

• Added some bindings for std::string_view when using c++17 #224 : ambiguous overloads
• Supports string_view for bind #355 : missing C++17 ifdef guards

I should probably add unit tests and examples using string_view.

[jagerman]

@dvtate - it would be nice to also see this solve #533 where string_view is not only nice to have but actually critically important for an application that wants to use secure memory for encrypted database keys.

[dvtate]

I just added functions to handle #533 but I'm getting a linker error with the unit tests. I need to sleep now but I can try and figure out what's going on with the unit test builds over the weekend.

[dvtate]

@jagerman , in order to maintain ABI compatibility and avoid the compiler errors that come with the ambiguity of const std::string_view vs. const std::string& (ie - implicit cast from string literal), the new key and rekey member functions should accept (const char* data, const size_t length). This also would benefit anyone who doesn't have C++17 yet. I'll adjust the code altho admittedly this might now be outside the scope of this PR.

To clarify, the linker error was caused by the library being compiled with an older C++ standard than the tests... which is something that we probably want to support.

[dvtate]

Just pushed those changes.

@SRombauts, does this look like something you think would make sense for the project? If not, please let me know and I'll close the PR.

[jagerman]

does this look like something you think would make sense for the project?

I know you didn't ask me, but personally I find this PR highly valuable: currently this library has no ability to bind TEXT values into statement placeholders without copying, which feels like a big omission versus the C API, which allows this easily.

I tried to work around the limitation by seeing if I could get the raw C API statement handle out of a SqliteCpp::Statement so that I could bind such values myself via the C API, but that proved impossible: Statement's internal handle is private and thus entirely inaccessible and so my choices were either: 1) avoid SQLite::Statement entirely and use the C API for all binding if I want to be able to do copy-less TEXT binding; 2) give up and just accept unnecessary copying; 3) fork the project to add it.

I opted for 3, but luckily found this PR already written and so am currently building on a fork with this merged.

(The key changes here are much less important, because Database does expose the C handle, so there it's pretty simple to just go to the C API for that one call).

[jagerman]

@jagerman , in order to maintain ABI compatibility and avoid the compiler errors that come with the ambiguity of const std::string_view vs. const std::string& (ie - implicit cast from string literal), the new key and rekey member functions should accept (const char* data, const size_t length). This also would benefit anyone who doesn't have C++17 yet. I'll adjust the code altho admittedly this might now be outside the scope of this PR.

I'm not sure if you would actually want to do this here or not, but one way to avoid that ambiguity is to define a C literal overload as well:

void key(const std::string& k) {(void)k;}
void key(const char* key) {(void)key;}
void key(std::string_view k) { (void) k; }

int main() {
    key("foo");
}

It is starting to feel a bit heavy though, and the pointer+size version would be perfectly fine for me.

[dvtate]

Agreed, but this library has been around for a while and idk how enthusiastic maintainers are about new features here.

I forgot that's an option. I personally like the pointer+len approach more because otherwise I'd have to convert my custom secure string type to std::string_view since it presumably could have '\0' characters... But I'd be happy to implement whatever approach makes reviewers happy.

[dvtate]

@SRombauts bump

[SRombauts]

Hey @dvtate, thanks a lot for the proposal and the bump!
I agree that we should really have this feature added, it's really a good idea.

I am sorry for the long delay, but sadly the PR was opened at a time when I was in vacation, the subject was too complex to wrap my head around, and then life happened 😓

Now, on the implementation, I see a few important issues that we should address before merging:

1. First (and I know it's my own fault for the delay), a rebase on master would be really needed.
2. There are too many changes: it would be much better to focus on just the original intent, and I am going fix Taking database key by const std::string& is insecure by design #533 in a separate PR -> Fix #533: accept binary database keys #571
3. We need to cover the code with unit tests.
4. We should discuss some of the changes related to ABI compat (it's broken now, not sure if it's really a big deal) and the C++11 / C++ 17 compat.

Let me know if you are willing to handle that, I would understand if it's too much work, esp. given how unresponsive I have been.

Sébastien

[dvtate]

Let me know if you are willing to handle that

Unless something comes up, I should have enough free time this week.

Knocking out 1-3 should be straightforward.

For 4, Because std::string_view is a C++17 feature I put those functions in #if blocks. This is done already for std::filesystem support, so I assumed this is okay. Only thing I can see getting in the way is implicit conversions.

Given that this is a minor, performance-oriented feature, vacation should definitely take priority IMO. Glad to see you're back tho :)

[dvtate]

Ok, I re-based my changes on master.

I'm seeing now that the library is built with C++11 so the string view methods don't get compiled. To avoid changing that I'll have to add in some C++11-compatible helper methods to isolate std::string_view to the headers.

[coveralls]

coverage: 97.956% (-1.8%) from 99.715% — dvtate:master into SRombauts:master

[SRombauts]

And there is the compatibility issue I was talking about: mixing c++11 & c++17. We want to be able to build the lib in one version, and use it in multiple clients with different version

Something that we should do for (or before) this task is adding a few of CI config to test :

1. build SQLiteCpp in c++11 but a "consummer" app (the tests or the example) in c++17
2. (the opposite is probably not an option)
3. both in c++17
4. both in c++11

In summary, it would mean perhaps just building 2 "consumers" apps with different c++ levels, to prevent any regression.

[SRombauts]

Unrelated, but, Column::getStringView() could probably be header-only using the existing getBlob() / getBytes() C++11 API.

But be careful with empty values: sqlite3_column_blob() may return nullptr, so you would need to return an empty std::string_view{} explicitly rather than constructing directly from the returned pointer, leading to undefined behaviro (I think) with string_view(nullptr, 0)

[dvtate]

Column::getStringView() could probably be header-only

Added that in commit.

I added some private Statement::bindText64 and Statement::bindTextNoCopy64 helpers to implement the string_view code to the headers. It seems that there's a lot of code that relies on implicit char* -> void* conversion for blobs, otherwise I would have made them public overloads.

With this change, the library can be built with C++11 and used with C++17 users. It appears the tests are built with C++17 as I'm seeing my string_view tests (within macros) passing.

It appears that std::string_view(nullptr, 0) is specified as UB... I'll fix that.

[SRombauts]

Thanks a lot for your hard work, I see that the CI is failing... I am going to review all that asap, either tonight or tomorrow.

I am actually considering removing support for C++11: I should release one last version and upgrade SQLiteCpp to a new version 4.0.0 with a more recent CMake requirement to support C++17 as the baseline!

This should make these changes simpler, and could help us modernize and simplify the codebase

[dvtate]

Oops, looking at the CI logs I forgot to put the column tests behind c++17 macros. I'll fix that.

[jagerman]

I should release one last version and upgrade SQLiteCpp to a new version 4.0.0 with a more recent CMake requirement to support C++17 as the baseline!

👍 on this idea: C++11 is pretty ancient at this point and C++17 is more or less available everywhere and brings a lot of nice language and STL improvements.

[jagerman]

Or to put it another way:

Version 3.0.0 - January 31 2020
C++11 is now required

That was 8 years 11 months after C++11 finalized. C++17 finalized 9 years 6 months ago. (And this makes me feel old).

[jagerman]

It appears that std::string_view(nullptr, 0) is specified as UB... I'll fix that.

That's not UB: that's exactly what a default-constructed string_view does. (It's construction from a nullptr without a size, or with a non-zero size, that are UB).

[dvtate]

Yea, as long as C++11 projects can still use the library it should be fine. C++11 is like Java 8 -- many are afraid to bump the number even if they don't have to use any of the new features.

Assuming the library and consumer are using the same/similar compilers everything should be fine afaik.

[dvtate]

@jagerman

That's not UB

Agreed that there's no reason for it to be UB but cppreference had some spooky wording and I'm not a language lawyer.

From what I'm seeing online, [(char*)nullptr, (char*)nullptr+0) is a valid empty range... so probably fine.

This SO thread suggests that it's not UB but a lot of the sources they reference relate to std::basic_string and not specifically std::basic_string_view.

So it's probably safe to remove the check. Unless there's a remote possibility that getBlob() returns nullptr and getBytes() != 0.

[SRombauts]

Regarding the potential UB I was referring to, the confusion was coming from the following:

std::string_view(nullptr, 0); // OK in C++17
std::string_view(nullptr);    // UB in C++17: tries traits::length(nullptr)

So you can just forget about it 🙏

[SRombauts]

I have been trying to do a thorough review of your code changes, that looks good overall, but I have had a few questions to dig deeper (including on some of my own codes that I couldn't remember why it was changed later, namely in Column::getString() where I have adjusted the comment on master)
I'll have to take more time later to write down a few minor comments

[dvtate]

Because of the code there I assumed I couldn't do

int len = getBytes();
return std::string_view((const char*) getBlob(), len);

Take your time reviewing.