[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: 99.727% (+0.01%) from 99.714% — 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.

[SRombauts]

Hello,

I am sorry but I made some more cleanup changes on the master branch that will require you to rebase again (or merge) and solve conflicts :(
The main goal of the recent changes was to make C++17 the baseline, so now we don't need (and shouldn't use) any "if cpp standard >=" checks!
See:

• Remove obsolete C++ compatibility guards #589
• Make C++17 the new baseline, and require CMake 3.16 #580
• Finish C++17 cleanup and verify minimum MSVC- #592

Sébastien

[SRombauts]

Ok, now for the long-awaited review: in summary, the overall API is good, but the implementation now need some cleanup IMO (please note that I can handle it if you don't want to tackle yet another change, I would understand perfectly!)

1. The #if __cplusplus >= 201703L blocks should all disappear as discussed above.
2. The old architectural motivation (that I suggested) for putting the C++17 functionality inline in the headers is gone: bindText64() and bindTextNoCopy64() should disappear entirely as they were there only for backward compat with C++11.
3. The only other important issue is that the default-constructed empty string_view is currently bound as SQL NULL, not empty TEXT:

with the current code:
bindText64(aIndex, aValue.data(), aValue.size());
doing that:
std::string_view value{};
is equivalent to:
sqlite3_bind_text64(stmt, index, nullptr, 0, ...);
this is isn't coherent with C++ semantic, where std::string_view a{} == std::string_view a{""}
I would implement both string-view bind functions using:
const char* data = aValue.empty() ? "" : aValue.data();

4. related to the "UB fix", the added code isn't in fact necessary:
   if (data == nullptr)
   return {};

5. bindNoCopy(std::string_view) should have a stronger lifetime warning like there is on bindNoCopy64()

The data must remain valid and unchanged until the parameter is rebound or the statement is finalized. Resetting the statement does not clear the binding.

6. Move getStringView() out of the header.

7. More unit tests (Note: that's also a place where I can do it myself after the merge if you prefer)

The most important cases aren't tested.
After the rebase I would specifically add coverage for:

std::string_view{}       // nullptr + size 0
std::string_view{""}     // non-null + size 0

for both bind() and bindNoCopy(), and verify that both produce empty TEXT, not NULL.

For getStringView(), test at least:

• normal TEXT;
• TEXT containing embedded NUL;
• BLOB containing embedded NUL;
• SQL NULL;
• zero-length BLOB;
• the existing UTF-16 database path.

[dvtate]

Wow, so sqlitecpp is completely dropping c++11 support. I didn't expect that.

I should be able to knock everything out either later today or tomorrow.

[SRombauts]

Yeah, sorry, it was perhaps drowned out a bit in the middle of a message last week:

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!

[dvtate]

I think this commit addresses all of your points altho it's possible I may have missed something in the tests.

I implemented getString in terms of getStringView in order to reduce duplication. The compiler will probably inline it to be the same thing.

Might make another PR after this to fix some typos in the documentation comments.

[dvtate]

removing support for C++11

I interpreted that as requiring that the library be built with C++17 but still usable by C++11 clients. This would let us move the string_view implementations out of the headers (and eliminate the helper functions) but would not eliminate the macros. I assumed C++11 support was super important.

[SRombauts]

I started the CI, let's see what it says, but I expect a Quality problem because of memcpy() usage with a nullptr:

memcmp(..., query.getColumn(3).getBlob(), sv1.size())

But memcmp does not permit either pointer argument to be null, even when the size is zero.

[SRombauts]

The coverage drop comes from the four untested named string_view overloads: 12 uncovered lines. The new implementation functions are covered.

If you can perhaps add a few test cases to please the CI, we should be good

Other than that, the good news is that the code looks good @dvtate 💯

[dvtate]

Ok I should have time to augment some of the tests to use those methods later today.

Edit: update: I've started but some things came up with work. Maybe later.

[dvtate]

Ok, sorry for the delay. I just added some code that tests those 4 functions. Hopefully that brings the coverage back to 100%.

[SRombauts]

Hello, sorry, the UB sanitizer detected a little mistake:

/home/runner/work/SQLiteCpp/SQLiteCpp/tests/Statement_test.cpp:477:9: runtime error: null pointer passed as argument 2, which is declared to never be null
/usr/include/string.h:65:33: note: nonnull attribute specified here
    #0 0x55ea2fb854eb in Statement_stringView_Test::TestBody()::$_0::operator()(bool) const /home/runner/work/SQLiteCpp/SQLiteCpp/tests/Statement_test.cpp:477:9
    #1 0x55ea2fb80b49 in Statement_stringView_Test::TestBody() /home/runner/work/SQLiteCpp/SQLiteCpp/tests/Statement_test.cpp:496:5
    #2 0x7f37d68ef81a in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/runner/work/SQLiteCpp/SQLiteCpp/googletest/googletest/src/gtest.cc:2698:10

[SRombauts]

Thanks a lot of the hard work!

[dvtate]

Glad we got this added!