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 bindNoCopy methods to allow binding std::string with SQLITE_STATIC #86

Closed

Conversation

DouglasHeriot
Copy link
Contributor

Should be safe, as long as you can guarantee the std::string exists while executing the query.

  • Added an accessor to Column that returns a std::string, that can handle BLOB or TEXT values that contain null-bytes.
  • Also more binding & Column cast support for uint32_t - fixes ambiguous overload errors when using unsigned-integer types.
    Note that I didn't use uint64_t, because unsigned 64-bit integers doesn't fit into SQLite (except for using int64_t and dealing with overflow with custom functions).
  • Added a C++11 move constructor to Statement, to allow storing it inside STL containers (eg. vector).
  • When binding, don’t pass int/double types as references – inefficient and unnecessary

Should be safe, as long as you can guarantee the std::string exists while executing the query.

Added an accessor to Column that returns a std::string, that can handle BLOB or TEXT values that contain null-bytes.

Also more binding & Column cast support for uint32_t - fixes ambiguous overload errors when using unsigned-integer types.
Note that I didn't use uint64_t, because unsigned 64-bit integers doesn't fit into SQLite (except for using int64_t and dealing with overflow with custom functions).

Added a C++11 move constructor to Statement, to allow storing it inside STL containers (eg. vector).
@SRombauts
Copy link
Owner

Hi, thanks, I agree with what you wrote and will review the code.

In the mean time, please have a look on the build failures.

Cheers!

@coveralls
Copy link

coveralls commented Jun 25, 2016

Coverage Status

Coverage increased (+16.3%) to 92.288% when pulling a3ea94c on DouglasHeriot:bindNoCopy into 50501a4 on SRombauts:master.

@DouglasHeriot
Copy link
Contributor Author

Fixed the build errors and cpplint style issues.
Might have a look at writing some more tests, especially covering edge-cases like dealing with null-bytes inside strings.

@SRombauts
Copy link
Owner

I did a first review of the code, looks great!

If you can add some unit tests it would be great. Else I hope to look into it in a few days.

@SRombauts
Copy link
Owner

Hi @DouglasHeriot, any news on unit tests for this?

If you want to do some, try to rebase to my master to get the new unit tests, so that you can complete them.

If you don't think you will have the time I can merge this and complete afterward.

Cheers!

@DouglasHeriot
Copy link
Contributor Author

Sorry, going to be a little while before I get back to this. Deadlines to meet over the next week, and then later I’ll try and come back and look at it.

SRombauts added a commit that referenced this pull request Jul 2, 2016
SRombauts added a commit that referenced this pull request Jul 2, 2016
 - see comments in code: needs a move constructor on Statement::Ptr
@SRombauts
Copy link
Owner

Totally understandable!

I will merge part of your work, and add some unit test.

I will not take the unfinished Statement move (see comments in code: needs a move constructor on Statement::Ptr). It needs some more work, and that would require even more unit tests that I am not able to do for now.

Cheers!

@SRombauts
Copy link
Owner

Hi @DouglasHeriot, did you see that I managed to add unit tests for all your methods?

Also, I removed the Statement move constructor as I believe it was incorrect.
If you need it, please provide a use case, even better with a use case.

In fact, to better exploit C++11 features we should rewrite the use of Statement::Ptr to wrap it in a std:::shared_ptr (hence the name).

@SRombauts
Copy link
Owner

Also, thanks a lot again for your work, it's always inspiring to get some help like that!

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

Successfully merging this pull request may close these issues.

3 participants