Skip to content
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.

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

Add binding mode option to database and database_binder #168

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

jimmyjxiao
Copy link
Contributor

@jimmyjxiao jimmyjxiao commented Jun 19, 2018

Allows the user to specify to use SQLite_static instead of transient all the time. In reference to @aminroosta comment about the interface on #167, I would argue this interface is better because it's more consistent with other things in c++ that use the << stream-like syntax. Iostream, for example, you can << an option and it'll affect things inputted in after.

For example:

std::cout  << "The number 42 in hex:     " << std::hex << 42 << '\n';

Output: The number 42 in hex: 2a

I think

db << "INSERT INTO foo values (?,?,?)" << binding_mode::do_not_copy << str1 << str2 << str3

Looks a lot cleaner than

db << "INSERT INTO foo values (?,?,?)" << binding_mode::do_not_copy(str1)<< binding_mode::do_not_copy(str2) << binding_mode::do_not_copy(str3)

At any rate, I decided to open a new pull request so we can discuss it here instead of on the old merged one.

@zauguin
Copy link
Collaborator

zauguin commented Jun 21, 2018

The modifier syntax looks quite nice when used inline, but they are dangerous when the statements are passed to different code.
If a function accepts a prepared statement as argument, it can't know the current binding mode and it also can't change it without modifying the outer state.

You could add a getter to inspect the value, but bugs in functions which do not expect this are hard to find: The code looks right and it works in almost all circumstances, except in the unusual case where a user is in do_not_copy mode.
Also the user has to guarantee the lifetime for every passed object, so adding an explicit marker ti every object makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants