Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Jun 19, 2018
+87 −52
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
64c40af
C++17
jimmyjxiao Jun 15, 2018
2f2e7d8
Fix compilation on MSVC/Windows
jimmyjxiao Jun 15, 2018
5515487
Fix Modern C++ detection on MSVC
jimmyjxiao Jun 15, 2018
fd3f0bf
String_view arguments instead of string
jimmyjxiao Jun 15, 2018
c69ffc1
Fallback to traditional string on pre-c++17 compilers
jimmyjxiao Jun 15, 2018
e0e3a22
Looks like String.h doesn't include string_view.h, maybe this will fi…
jimmyjxiao Jun 15, 2018
6c95d0a
Fix preprocessor directives
jimmyjxiao Jun 15, 2018
3b181fe
has_sqlite_type specifically for stringviews is unnecessary, it looks…
jimmyjxiao Jun 15, 2018
b9ddb6b
Original SQL should have string, not string_view return type.
jimmyjxiao Jun 17, 2018
b08f6b6
Reuse UTF-8 prepare function, and add length to prepare call
jimmyjxiao Jun 17, 2018
080411a
Switch back to string parameters for database constructor
jimmyjxiao Jun 17, 2018
64f5f66
Function definitions back to std::string
jimmyjxiao Jun 17, 2018
9e56736
Sizes for string_view functions
jimmyjxiao Jun 17, 2018
a1849e2
Change preprocessor options for string_view to be more consistent wit…
jimmyjxiao Jun 17, 2018
fac3344
Switch to typedef instead of define
jimmyjxiao Jun 17, 2018
8e09803
Pass string_view by value
jimmyjxiao Jun 17, 2018
7d6ea22
Small Changes
jimmyjxiao Jun 18, 2018
c03e4e8
Add tests from string_view
jimmyjxiao Jun 18, 2018
b12f949
Update Readme
jimmyjxiao Jun 18, 2018
dffc090
oops
jimmyjxiao Jun 18, 2018
b0e3c8d
Sqlite takes bind text16 sizes in bytes
jimmyjxiao Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake")
include("cmake/HunterGate.cmake")
Expand Down Expand Up @@ -43,6 +43,7 @@ else()
endif()

catch_discover_tests(tests)
target_compile_options(tests PUBLIC $<$<CXX_COMPILER_ID:MSVC>:/Zc:__cplusplus> )

# Place the file in the source directory, permitting us to place a single configuration file for YCM there.
# YCM is the code-completion engine for (neo)vim https://github.com/Valloric/YouCompleteMe
Expand Down
42 changes: 26 additions & 16 deletions hdr/sqlite_modern_cpp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,19 @@ namespace sqlite {
return ++_inx;
}

sqlite3_stmt* _prepare(const std::u16string& sql) {
return _prepare(utility::utf16_to_utf8(sql));
sqlite3_stmt* _prepare(const U16STR_REF& sql) {
//return _prepare(utility::utf16_to_utf8(sql));
int hresult;
sqlite3_stmt* tmp = nullptr;
const void *remaining;
hresult = sqlite3_prepare16_v2(_db.get(), sql.data(), -1, &tmp, &remaining);
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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

sqlite3_stmt* _prepare(const std::string& sql) {
sqlite3_stmt* _prepare(const STR_REF& sql) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

int hresult;
sqlite3_stmt* tmp = nullptr;
const char *remaining;

This comment was marked as resolved.

Sign in to view

Expand All @@ -105,13 +113,13 @@ namespace sqlite {

public:

database_binder(std::shared_ptr<sqlite3> db, std::u16string const & sql):
database_binder(std::shared_ptr<sqlite3> db, U16STR_REF const & sql):
_db(db),
_stmt(_prepare(sql), sqlite3_finalize),
_inx(0) {
}

database_binder(std::shared_ptr<sqlite3> db, std::string const & sql):
database_binder(std::shared_ptr<sqlite3> db, STR_REF const & sql):
_db(db),
_stmt(_prepare(sql), sqlite3_finalize),
_inx(0) {
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

_db = std::shared_ptr<sqlite3>(tmp, [=](sqlite3* ptr) { sqlite3_close_v2(ptr); }); // this will close the connection eventually when no longer needed.
Expand All @@ -372,8 +380,8 @@ namespace sqlite {
*this << R"(PRAGMA encoding = "UTF-16";)";
}

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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem as above.

sqlite3* tmp = nullptr;
auto ret = sqlite3_open_v2(db_name_utf8.data(), &tmp, static_cast<int>(config.flags), config.zVfs);
_db = std::shared_ptr<sqlite3>(tmp, [=](sqlite3* ptr) { sqlite3_close_v2(ptr); }); // this will close the connection eventually when no longer needed.
Expand All @@ -386,20 +394,20 @@ namespace sqlite {
database(std::shared_ptr<sqlite3> db):
_db(db) {}

database_binder operator<<(const std::string& sql) {
database_binder operator<<(const STR_REF& sql) {
return database_binder(_db, sql);
}

database_binder operator<<(const char* sql) {
return *this << std::string(sql);
return *this << STR_REF(sql);
}

database_binder operator<<(const std::u16string& sql) {
database_binder operator<<(const U16STR_REF& sql) {
return database_binder(_db, sql);
}

database_binder operator<<(const char16_t* sql) {
return *this << std::u16string(sql);
return *this << U16STR_REF(sql);
}

connection_type connection() const { return _db; }
Expand All @@ -413,12 +421,12 @@ namespace sqlite {
}

template <typename Function>
void define(const std::string &name, Function&& func) {
void define(const STR_REF &name, Function&& func) {
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,

This comment was marked as resolved.

Sign in to view

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sql_function_binder::scalar<traits::arity, typename std::remove_reference<Function>::type>,
nullptr, nullptr, [](void* ptr){
delete static_cast<decltype(funcPtr)>(ptr);
Expand All @@ -427,13 +435,13 @@ namespace sqlite {
}

template <typename StepFunction, typename FinalFunction>
void define(const std::string &name, StepFunction&& step, FinalFunction&& final) {
void define(const STR_REF &name, StepFunction&& step, FinalFunction&& final) {
typedef utility::function_traits<StepFunction> traits;
using ContextType = typename std::remove_reference<typename traits::template argument<0>>::type;

auto funcPtr = new auto(std::make_pair(std::forward<StepFunction>(step), std::forward<FinalFunction>(final)));
if(int result = sqlite3_create_function_v2(
_db.get(), name.c_str(), traits::arity - 1, SQLITE_UTF8, funcPtr, nullptr,
_db.get(), name.data(), traits::arity - 1, SQLITE_UTF8, funcPtr, nullptr,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

sql_function_binder::step<ContextType, traits::arity, typename std::remove_reference<decltype(*funcPtr)>::type>,
sql_function_binder::final<ContextType, typename std::remove_reference<decltype(*funcPtr)>::type>,
[](void* ptr){
Expand Down Expand Up @@ -652,3 +660,5 @@ namespace sqlite {
}
}
}
#undef STR_REF
#undef U16STR_REF
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the newline at the end of the file.
Also undefining them here seems odd because they are not defined in this file.
This is not a problem for typedefs.

6 changes: 3 additions & 3 deletions hdr/sqlite_modern_cpp/errors.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ namespace sqlite {

class sqlite_exception: public std::runtime_error {
public:
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)) {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

int get_code() const {return code & 0xFF;}
int get_extended_code() const {return code;}
std::string get_sql() const {return sql;}
Expand Down Expand Up @@ -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 = "") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is still a const&

switch(error_code & 0xFF) {
#define SQLITE_MODERN_CPP_ERROR_CODE(NAME,name,derived) \
case SQLITE_ ## NAME: switch(error_code) { \
Expand Down
4 changes: 2 additions & 2 deletions hdr/sqlite_modern_cpp/log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ namespace sqlite {
}
});

sqlite3_config(SQLITE_CONFIG_LOG, (void(*)(void*,int,const char*))[](void *functor, int error_code, const char *errstr) {
sqlite3_config(SQLITE_CONFIG_LOG, static_cast<void(*)(void*,int,const char*)>([](void *functor, int error_code, const char *errstr) {
(*static_cast<decltype(ptr.get())>(functor))(error_code, errstr);
}, ptr.get());
}), ptr.get());
detail::store_error_log_data_pointer(std::move(ptr));
}
}
39 changes: 25 additions & 14 deletions hdr/sqlite_modern_cpp/type_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,20 @@
#include <string>
#include <memory>
#include <vector>

#ifdef __has_include
#if __cplusplus >= 201703 && __has_include(<string_view>)
#define MODERN_SQLITE_STRINGVIEW_SUPPORT
#include <string_view>
#define STR_REF std::string_view
#define U16STR_REF std::u16string_view
#else
#define STR_REF std::string
#define U16STR_REF std::u16string
#endif
#else
#define STR_REF std::string
#define U16STR_REF std::u16string
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more modern and more robust to use typedefs instead of macros. It is also more consistant with optional etc.

#ifdef __has_include
#if __cplusplus > 201402 && __has_include(<optional>)
#define MODERN_SQLITE_STD_OPTIONAL_SUPPORT
Expand Down Expand Up @@ -150,16 +163,15 @@ namespace sqlite {
sqlite3_result_null(db);
}

// std::string
// STR_REF
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 has to be replaced with the actual length.

}

// Convert char* to string to trigger op<<(..., const std::string )
template<std::size_t N> inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const char(&STR)[N]) { return bind_col_in_db(stmt, inx, std::string(STR, N-1)); }
// Convert char* to string_view to trigger op<<(..., const STR_REF )
template<std::size_t N> inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const char(&STR)[N]) { return bind_col_in_db(stmt, inx, STR_REF(STR, N-1)); }

inline std::string get_col_from_db(sqlite3_stmt* stmt, int inx, result_type<std::string>) {
return sqlite3_column_type(stmt, inx) == SQLITE_NULL ? std::string() :
Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 has to be replaced with the actual length.

}
// std::u16string
// U16STR_REF
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 has to be replaced with the actual length.

}

// Convert char* to string to trigger op<<(..., const std::string )
template<std::size_t N> inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const char16_t(&STR)[N]) { return bind_col_in_db(stmt, inx, std::u16string(STR, N-1)); }
// Convert char* to string_view to trigger op<<(..., const STR_REF )
template<std::size_t N> inline int bind_col_in_db(sqlite3_stmt* stmt, int inx, const char16_t(&STR)[N]) { return bind_col_in_db(stmt, inx, U16STR_REF(STR, N-1)); }

inline std::u16string get_col_from_db(sqlite3_stmt* stmt, int inx, result_type<std::u16string>) {
return sqlite3_column_type(stmt, inx) == SQLITE_NULL ? std::u16string() :
std::u16string(reinterpret_cast<char16_t const *>(sqlite3_column_text16(stmt, inx)), sqlite3_column_bytes16(stmt, inx));
}
inline std::u16string get_val_from_db(sqlite3_value *value, result_type<std::u16string >) {
inline std::u16string get_val_from_db(sqlite3_value *value, result_type<std::u16string>) {
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 has to be replaced with the actual length.

}

Expand Down
5 changes: 3 additions & 2 deletions tests/flags.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ struct TmpFile {
TmpFile(): fname("./flags.db") { }
~TmpFile() { remove(fname.c_str()); }
};

#if BYTE_ORDER == BIG_ENDIAN
#ifdef _WIN32
#define OUR_UTF16 "UTF-16le"
#elif BYTE_ORDER == BIG_ENDIAN
#define OUR_UTF16 "UTF-16be"
#else
#define OUR_UTF16 "UTF-16le"
Expand Down