search

SOCI / soci

Official repository of the SOCI - The C++ Database Access Library
http://soci.sourceforge.net/
Boost Software License 1.0
1.41k stars 477 forks source link

allow move semantics for session class #1096

Closed olegendo closed 11 months ago

vadz commented 11 months ago

Sorry but this is very obviously incorrect, e.g. because backEnd_ is going to be deleted twice.

Making session movable should be possible, but it isn't as simple as this.

Also, please always add unit tests for the new functionality as any such test would have caught the problem mentioned above immediately. Thanks in advance!

olegendo commented 11 months ago

@vadz Thanks for looking at it. Sorry, yes, I didn't check this obvious issue. Let me try again ... (will send another pull request).

Regarding test case, I'm having a bit of a difficulty navigating that. Where would it go? I've tried running "make test" and it doesn't catch the bug. But then, it also fails 10 tests out of 14 here ...

vadz commented 11 months ago

Most of the tests are in tests/common-tests.h, so this one should be added there.

It's not normal that some of the tests fail, if the reason for this is not obvious (e.g. wrong database connection), then please try to debug this and/or open separate issues for the failures. But all the existing tests are supposed to pass and just adding the move ctor is not going to change anything because it is not used in any of them (because it doesn't currently exist!). You need to add a test case for this, moving both empty and valid session objects.