Use std::string_view as argument instead of std::string on C++17 Compilers #167

jimmyjxiao · 2018-06-15T21:43:56Z

The major benefit is if someone tries to bind a char* parameter, we can avoid an extra copy that would happen if a new std::string was created from it. This pull request also includes some improvements for building/running the tests on MSVC, mostly because I suck at git and didn't know how to separate them into a separate pull request.

…x travis ci build?

… like

aminroosta

Looks good to me 👍
@zowpowow please update the Readme

zauguin · 2018-06-17T07:53:53Z

Thanks for the pull request!

While this works great for users passing literals and std::strings, some changes are needed to also support not null-terminated string_views.
Details inline.

zauguin · 2018-06-17T07:55:01Z

hdr/sqlite_modern_cpp.h

+			if (hresult != SQLITE_OK) errors::throw_sqlite_error(hresult, utility::utf16_to_utf8(sql.data()));
+			if (!std::all_of(static_cast<const char16_t*>(remaining), sql.data() + sql.size(), [](char16_t ch) {return std::isspace(ch); }))
+				throw errors::more_statements("Multiple semicolon separated statements are unsupported", utility::utf16_to_utf8(sql.data()));
+			return tmp;


Is there an advantage in not reusing the UTF-8 version?
I see some smaller problems here:

The SQL statement in the exception is no longer the same statement given to SQLite.
In edge cases utf16_to_utf8 might behave differently than SQLite which can make debugging hard.

Less usage of utf16_to_utf8 in main code paths might lead to problems in this function to go unnoticed which again makes debugging a nightmare if they only surface at error conditions.

It is marginally slower because in case of an error the conversion happens twice.

And of course we introduce some redundant code.

zauguin

To avoid similar problems it would be nice to add some tests.

zauguin · 2018-06-17T07:56:19Z

hdr/sqlite_modern_cpp.h

 		}

-		sqlite3_stmt* _prepare(const std::string& sql) {
+		sqlite3_stmt* _prepare(const STR_REF& sql) {


We should accept a string_view by value instead of const reference. This applies to all functions.

Can I ask what the advantage of this would be? I know that in my use case, I'm going to have a few string_view objects already around and wouldn't passing by const reference be marginally faster than passing by value?

In most cases they will be inlined, then it doesn't make a difference. Otherwise the advantage is that a const reference requires some object to reference, so the string_view has to exists in memory(typically on the stack). But when passing by value small objects like this normally are passed in registers which does not require any usage of (slow) memory.
Most of the time the string_view will not exist in memory before(because it is a temporary which did not exists or because the optimizer keeps the values in registers), so copying 2*sizeof(void*) between registers should be much faster than writing this into memory, copying the pointer and reading from memory again.
This does mostly apply for the Itanium ABI, so not on Windows. But output from Compiler-Explorer suggests that passing by value saves a memory access on Windows too.
Additionally the interface looks cleaner 😉

Disclaimer: I did not run a benchmark on this so I could be wrong. In this case, I would love to hear why.

You're probably right. I wrote some super crappy benchmarks and it seems passing by string_view was slightly faster: https://github.com/zowpowow/sqlite_modern_cpp/tree/str_view_by_value

hdr/sqlite_modern_cpp.h

 		}

-		sqlite3_stmt* _prepare(const std::string& sql) {
+		sqlite3_stmt* _prepare(const STR_REF& sql) {
 			int hresult;
 			sqlite3_stmt* tmp = nullptr;
 			const char *remaining;


zauguin · 2018-06-17T08:01:01Z

hdr/sqlite_modern_cpp.h

@@ -362,7 +370,7 @@ namespace sqlite {
 		std::shared_ptr<sqlite3> _db;

 	public:
-		database(const std::string &db_name, const sqlite_config &config = {}): _db(nullptr) {
+		database(const STR_REF &db_name, const sqlite_config &config = {}): _db(nullptr) {
 			sqlite3* tmp = nullptr;
 			auto ret = sqlite3_open_v2(db_name.data(), &tmp, static_cast<int>(config.flags), config.zVfs);


This assumes that db_name is null-terminated. The db_name ends up as a filenamem, so not null-terminated strings are not supported here.
Maybe we should keep std::string here and optionally add a const char * overload?

zauguin · 2018-06-17T08:02:40Z

hdr/sqlite_modern_cpp.h

-		database(const std::u16string &db_name, const sqlite_config &config = {}): _db(nullptr) {
-			auto db_name_utf8 = utility::utf16_to_utf8(db_name);
+		database(const U16STR_REF &db_name, const sqlite_config &config = {}): _db(nullptr) {
+			auto db_name_utf8 = utility::utf16_to_utf8(db_name.data());


Same problem as above.

zauguin · 2018-06-18T10:04:15Z

hdr/sqlite_modern_cpp.h

 			typedef utility::function_traits<Function> traits;

 			auto funcPtr = new auto(std::forward<Function>(func));
 			if(int result = sqlite3_create_function_v2(
-					_db.get(), name.c_str(), traits::arity, SQLITE_UTF8, funcPtr,
+					_db.get(), name.data(), traits::arity, SQLITE_UTF8, funcPtr,


I think we should stick to .c_str() here because it makes clearer that we really need a c-style string, especially the trailing nul byte.

zauguin · 2018-06-17T08:10:23Z

hdr/sqlite_modern_cpp/type_wrapper.h

 	template<>
 	struct has_sqlite_type<std::string, SQLITE3_TEXT, void> : std::true_type {};
-
-	inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const std::string& val) {
+	inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const STR_REF& val) {
 		return sqlite3_bind_text(stmt, inx, val.data(), -1, SQLITE_TRANSIENT);


-1 has to be replaced with the actual length.

zauguin · 2018-06-17T08:11:24Z

hdr/sqlite_modern_cpp/type_wrapper.h

@@ -170,30 +182,29 @@ namespace sqlite {
 			std::string(reinterpret_cast<char const *>(sqlite3_value_text(value)), sqlite3_value_bytes(value));
 	}

-	inline void store_result_in_db(sqlite3_context* db, const std::string& val) {
+	inline void store_result_in_db(sqlite3_context* db, const STR_REF& val) {
 		sqlite3_result_text(db, val.data(), -1, SQLITE_TRANSIENT);


-1 has to be replaced with the actual length.

zauguin · 2018-06-17T08:11:47Z

hdr/sqlite_modern_cpp/type_wrapper.h

 	template<>
 	struct has_sqlite_type<std::u16string, SQLITE3_TEXT, void> : std::true_type {};
-
-	inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const std::u16string& val) {
+	inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const U16STR_REF& val) {
 		return sqlite3_bind_text16(stmt, inx, val.data(), -1, SQLITE_TRANSIENT);


-1 has to be replaced with the actual length.

zauguin · 2018-06-17T08:12:30Z

hdr/sqlite_modern_cpp/type_wrapper.h

 		return sqlite3_value_type(value) == SQLITE_NULL ? std::u16string() :
 			std::u16string(reinterpret_cast<char16_t const *>(sqlite3_value_text16(value)), sqlite3_value_bytes16(value));
 	}

-	inline void store_result_in_db(sqlite3_context* db, const std::u16string& val) {
+	inline void store_result_in_db(sqlite3_context* db, const U16STR_REF& val) {
 		sqlite3_result_text16(db, val.data(), -1, SQLITE_TRANSIENT);


-1 has to be replaced with the actual length.

zauguin · 2018-06-17T08:17:24Z

CMakeLists.txt

@@ -3,7 +3,7 @@ OPTION(ENABLE_SQLCIPHER_TESTS "enable sqlchipher test")

 # Creates the file compile_commands.json in the build directory.
 SET(CMAKE_EXPORT_COMPILE_COMMANDS ON)
-set (CMAKE_CXX_STANDARD 14)
+set (CMAKE_CXX_STANDARD 17)


We still support C++14, so it would make sense to keep this set to 14. Especially since our Travis compiler is so old that it does not support most C++17 features anyway.

Also got rid of some redundant code

…h other options Also makes it so people can manually define MODERN_SQLITE_STRINGVIEW_SUPPORT if they want

zauguin · 2018-06-18T07:21:44Z

Nice. Updating the ReadMe and adding a test would be great. There are some non-functional changes left but these are details.

It is quite sad that this doesn't really work out for function and database names. These should mostly be covered by small string optimization anyway but we could overload them for C-strings to avoid temporary strings at least when a literal is given? After merging this we might want a separate Pull Request in that direction.

zauguin

Some smaller possible changes

zauguin · 2018-06-18T09:55:56Z

hdr/sqlite_modern_cpp.h

-			if(ret != SQLITE_OK) errors::throw_sqlite_error(_db ? sqlite3_extended_errcode(_db.get()) : ret);
-			sqlite3_extended_result_codes(_db.get(), true);
-			if(config.encoding != Encoding::UTF8)
+		database(const std::u16string &db_name, const sqlite_config &config = {}): database(utility::utf16_to_utf8(db_name.data()), config) {


.data() is useless here and causes a unnecessary strlen

zauguin · 2018-06-18T10:04:15Z

hdr/sqlite_modern_cpp.h

 			typedef utility::function_traits<Function> traits;

 			auto funcPtr = new auto(std::forward<Function>(func));
 			if(int result = sqlite3_create_function_v2(
-					_db.get(), name.c_str(), traits::arity, SQLITE_UTF8, funcPtr,
+					_db.get(), name.data(), traits::arity, SQLITE_UTF8, funcPtr,


I think we should stick to .c_str() here because it makes clearer that we really need a c-style string, especially the trailing nul byte.

zauguin · 2018-06-18T10:07:31Z

hdr/sqlite_modern_cpp/errors.h

-		sqlite_exception(const char* msg, std::string sql, int code = -1): runtime_error(msg), code(code), sql(sql) {}
-		sqlite_exception(int code, std::string sql): runtime_error(sqlite3_errstr(code)), code(code), sql(sql) {}
+		sqlite_exception(const char* msg, STR_REF sql, int code = -1): runtime_error(msg), code(code), sql(std::move(sql)) {}
+		sqlite_exception(int code, STR_REF sql): runtime_error(sqlite3_errstr(code)), code(code), sql(std::move(sql)) {}


The std::move is useless ere, moving from a const reference or a string_view is just copying.

zauguin · 2018-06-18T10:07:50Z

hdr/sqlite_modern_cpp/errors.h

@@ -40,7 +40,7 @@ namespace sqlite {
 		class more_statements: public sqlite_exception { using sqlite_exception::sqlite_exception; }; // Prepared statements can only contain one statement
 		class invalid_utf16: public sqlite_exception { using sqlite_exception::sqlite_exception; };

-		static void throw_sqlite_error(const int& error_code, const std::string &sql = "") {
+		static void throw_sqlite_error(const int& error_code, const STR_REF &sql = "") {


Here is still a const&

zauguin · 2018-06-18T10:09:57Z

hdr/sqlite_modern_cpp/type_wrapper.h

+#else
+typedef const std::string& STR_REF;
+typedef const std::u16string& U16STR_REF;
+#endif


The typedef should be in out namespace to avoid introducing names in the global namespace. Also they are no longer macros, so the names should no longer be uppercase.

zauguin · 2018-06-18T10:12:07Z

hdr/sqlite_modern_cpp/utility/utf16_utf8.h

@@ -8,7 +8,7 @@

 namespace sqlite {
 	namespace utility {
-		inline std::string utf16_to_utf8(const std::u16string &input) {
+		inline std::string utf16_to_utf8(const U16STR_REF &input) {


Another const&.

jimmyjxiao · 2018-06-18T15:43:13Z

While we're on the topic of avoiding extra copies, is SQLITE_TRANSIENT really necessary over SQLITE_STATIC?

zauguin · 2018-06-18T16:05:55Z

If we know we have a literal string we could use SQLITE_STATIC but when the user gives us a std::string(_view), we can not know how long it is valid. For example using the iterator interface or prepared statements, passed temporarys will very often be destroyed before the statement is complete.
We could document the requirements and provide optional overloads for SQLITE_STATIC but I think the reduced safety is not worth the probably rather small performance advantage. Especially because I do not hink there is a way to detect errors here without something like valgrind and in most cases the database disk write will be significantly more expensive than the string copy anyway.

Of course, I would be happy to hear about ideas how this could work.

jimmyjxiao · 2018-06-18T18:29:04Z

Alright, Readme and tests have been updated, can we take another look at merging? Also for the SQLITE_STATIC thing, take a look here: jimmyjxiao@5d5ce07 on how I think it could be implemented

aminroosta · 2018-06-19T00:51:47Z

@zowpowow About SQLITE_STATIC, @5d5ce07 looks promising.
However i'm more worried about the interface, instead of having

db << "select * where name = ?" << binding_mode::static << "tom";

I'd recommend

db << "select * where name = ?" << sqlite::as_static("tom");

Please see #122 , we have a discussion on supporting blob types without vector<T>,
The idea was the the same to have << sqlite::blob(void*, size_t) overloaded.

jimmyjxiao added 9 commits June 15, 2018 11:21

C++17

64c40af

Fix compilation on MSVC/Windows

2f2e7d8

Fix Modern C++ detection on MSVC

5515487

String_view arguments instead of string

fd3f0bf

Fallback to traditional string on pre-c++17 compilers

c69ffc1

Looks like String.h doesn't include string_view.h, maybe this will fi…

e0e3a22

…x travis ci build?

Fix preprocessor directives

6c95d0a

has_sqlite_type specifically for stringviews is unnecessary, it looks…

3b181fe

… like

Original SQL should have string, not string_view return type.

b9ddb6b

aminroosta approved these changes Jun 17, 2018

View reviewed changes

aminroosta requested a review from zauguin June 17, 2018 04:48

zauguin reviewed Jun 17, 2018

View reviewed changes

zauguin requested changes Jun 17, 2018

View reviewed changes

jimmyjxiao added 7 commits June 17, 2018 10:06

Reuse UTF-8 prepare function, and add length to prepare call

b08f6b6

Switch back to string parameters for database constructor

080411a

Also got rid of some redundant code

Function definitions back to std::string

64f5f66

Sizes for string_view functions

9e56736

Change preprocessor options for string_view to be more consistent wit…

a1849e2

…h other options Also makes it so people can manually define MODERN_SQLITE_STRINGVIEW_SUPPORT if they want

Switch to typedef instead of define

fac3344

Pass string_view by value

8e09803

zauguin reviewed Jun 18, 2018

View reviewed changes

Small Changes

7d6ea22

jimmyjxiao added 4 commits June 18, 2018 10:20

Add tests from string_view

c03e4e8

Update Readme

b12f949

oops

dffc090

Sqlite takes bind text16 sizes in bytes

b0e3c8d

zauguin approved these changes Jun 18, 2018

View reviewed changes

aminroosta merged commit 8327ea6 into SqliteModernCpp:dev Jun 19, 2018

jimmyjxiao mentioned this pull request Jun 19, 2018

Add binding mode option to database and database_binder #168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use std::string_view as argument instead of std::string on C++17 Compilers #167

Use std::string_view as argument instead of std::string on C++17 Compilers #167

jimmyjxiao commented Jun 15, 2018

aminroosta left a comment

zauguin commented Jun 17, 2018

zauguin Jun 17, 2018

zauguin left a comment

zauguin Jun 17, 2018

jimmyjxiao Jun 17, 2018

zauguin Jun 17, 2018

jimmyjxiao Jun 17, 2018

This comment was marked as resolved.

zauguin Jun 17, 2018

zauguin Jun 17, 2018

This comment was marked as resolved.

zauguin Jun 18, 2018

zauguin Jun 17, 2018

zauguin Jun 17, 2018

zauguin Jun 17, 2018

zauguin Jun 17, 2018

zauguin Jun 17, 2018

zauguin commented Jun 18, 2018

zauguin left a comment

zauguin Jun 18, 2018

zauguin Jun 18, 2018

zauguin Jun 18, 2018

zauguin Jun 18, 2018

zauguin Jun 18, 2018

zauguin Jun 18, 2018

jimmyjxiao commented Jun 18, 2018

zauguin commented Jun 18, 2018

jimmyjxiao commented Jun 18, 2018

aminroosta commented Jun 19, 2018 •

edited

Loading

Use std::string_view as argument instead of std::string on C++17 Compilers #167

Use std::string_view as argument instead of std::string on C++17 Compilers #167

Conversation

jimmyjxiao commented Jun 15, 2018

aminroosta left a comment

Choose a reason for hiding this comment

zauguin commented Jun 17, 2018

Choose a reason for hiding this comment

zauguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zauguin commented Jun 18, 2018

zauguin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyjxiao commented Jun 18, 2018

zauguin commented Jun 18, 2018

jimmyjxiao commented Jun 18, 2018

aminroosta commented Jun 19, 2018 • edited Loading

aminroosta commented Jun 19, 2018 •

edited

Loading