make session and connection_parameters movable.

olegendo commented 8 months ago

This is a second try of https://github.com/SOCI/soci/pull/1096

Although it seems not absolutely necessary, I've also made connection_parameters movable
The logger inside class session could also be made movable but that was a bit difficult because it doesn't support impl = nullptr case. So I've left it alone for now and it should resort to making a clone/copy.
I've added a test case. Not sure if it's in the right or wrong place though. The test coverage might be not excessive, but it does catch the error (segfault here) when default move ctor / operator= is used for class session, as I had it originally.

olegendo commented 8 months ago

Please let me know if you could update the PR to do this. TIA!

Yes, of course. I will force-push an updated version after we've cleared out the open points.

vadz commented 8 months ago

Please let me know if you could update the PR to do this. TIA!

Yes, of course. I will force-push an updated version after we've cleared out the open points.

I think I've answered all of them, please let me know if I've missed anything. And TIA!

olegendo commented 8 months ago

I think I've answered all of them, please let me know if I've missed anything. And TIA!

I've decided to make a separate follow-up commit for sake of clarity. Please squash them during merge later. Let me know if there's anything else to be addressed.

olegendo commented 8 months ago

Oh ouch. The self-move test case fails to build in the CI

/tmp/cirrus-ci-build/tests/common-tests.h:4341:15: error: explicitly moving variable of type 'soci::session' to itself [-Werror,-Wself-move]
        sql_1 = std::move(sql_1);
        ~~~~~ ^           ~~~~~

How to fix it?

vadz commented 8 months ago

Oh ouch. The self-move test case fails to build in the CI

/tmp/cirrus-ci-build/tests/common-tests.h:4341:15: error: explicitly moving variable of type 'soci::session' to itself [-Werror,-Wself-move]
        sql_1 = std::move(sql_1);
        ~~~~~ ^           ~~~~~

How to fix it?

We need to use SOCI_GCC_WARNING_SUPPRESS(self-move) before (and SOCI_GCC_WARNING_RESTORE() after) it, see the existing uses of these macros.

olegendo commented 8 months ago

We need to use SOCI_GCC_WARNING_SUPPRESS(self-move) before (and SOCI_GCC_WARNING_RESTORE() after) it, see the existing uses of these macros.

It'd be the first place to use SOCI_GCC_WARNING_SUPPRESS(self-move). It seems this particular warning can't be pragma'd away:

In file included from .../soci/tests/mysql/test-mysql.cpp:11:
.../soci/tests/common-tests.h: In member function ‘void soci::tests::test_cases::{anonymous}::C_A_T_C_H_T_E_S_T_130::test()’:
.../soci/include/private/soci-compiler.h:31:9: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
   31 |         _Pragma (SOCI_STRINGIZE(GCC diagnostic ignored SOCI_STRINGIZE(SOCI_CONCAT(-W,x))))
      |         ^~~~~~~
.../soci/tests/common-tests.h:4325:9: note: in expansion of macro ‘SOCI_GCC_WARNING_SUPPRESS’
 4325 |         SOCI_GCC_WARNING_SUPPRESS(self-move)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~

We'd have to silence it by using the -Wno-self-move compiler option when building the tests. How about that?


diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index 7e65d2c6..2daa7cf2 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -14,6 +14,11 @@ colormsg(_HIBLUE_ "Configuring SOCI tests:")
 # This works around a problem when building in C++11 mode with clang (see #984).
 add_definitions(-DCATCH_CONFIG_CPP11_NO_SHUFFLE)

+if(MSVC)
+else()
+  add_compile_options(-Wno-self-move)
+endif()
+
 include_directories(
   ${SOCI_SOURCE_DIR}/include/private
   ${CMAKE_CURRENT_SOURCE_DIR})

vadz commented 8 months ago

It'd be the first place to use SOCI_GCC_WARNING_SUPPRESS(self-move). It seems this particular warning can't be pragma'd away:

It can, or at least I don't see why wouldn't it, it's just that this seems to be a clang-only warning, so we need to add SOCI_CLANG_WARNING_SUPPRESS() for it. Could you please add it reusing the code from here:

https://github.com/wxWidgets/wxWidgets/blob/dc6a0c069bdde714f22cb0459efd9ad186ce7179/include/wx/defs.h#L671-L692

?

olegendo commented 8 months ago

It can, or at least I don't see why wouldn't it,

See the compiler messages above which I get when I try to use the diagnostics pragma with 'self-move'. I'm using GCC 12, not clang.

vadz commented 8 months ago

It can, or at least I don't see why wouldn't it,

See the compiler messages above which I get when I try to use the diagnostics pragma with 'self-move'. I'm using GCC 12, not clang.

Yes, which is why the pragma is not recognized -- gcc doesn't have such warning, so it can't be suppressed for it. If you only suppress it for clang, things will work for both compilers.

olegendo commented 8 months ago

Yes, which is why the pragma is not recognized -- gcc doesn't have such warning, so it can't be suppressed for it. If you only suppress it for clang, things will work for both compilers.

Just checked again, -Wself-move was added in GCC 13. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159 So if this is done as a clang-only pragma, it will probably fall apart in 1~2 years when GCC is updated (if tests are built with -Wall, which seems to be the case )

It looks like if -Wno-self-move global compiler option is specified GCC 12 will not complain about it. So I guess when used on GCC 13 and clang it will yield the desired result -- just that you won't have the fine grained control over that option for each test case.

I'm approaching the time limit that I can spend on this issue. I've just added a commit to use the global option to disable the warning but left the SOCI_GCC_WARNING_SUPPRESS(self-move) as comments in place. So when there is a better way to handle this, it should be easier to change.

BTW, the other movable thing row also doesn't have the self-move test.

olegendo commented 8 months ago

It looks like if -Wno-self-move global compiler option is specified GCC 12 will not complain about it. So I guess when used on GCC 13 and clang it will yield the desired result -- just that you won't have the fine grained control over that option for each test case.

That seems also not to be always the case. One of the CI jobs fails due to:

cc1plus.exe: error: unrecognized command line option '-Wno-self-move' [-Werror] Perhaps it's better to omit this test case entirely for now. IMO it's causing more work and headaches than it's worth it. The self-move case is handled explicitly in the session code.

olegendo commented 8 months ago

I've added a compiler version check around the SOCI_GCC_WARNING_SUPPRESS(self-move). Please check again if this is OK.

olegendo commented 8 months ago

Anything else that needs to be done for this one?

vadz commented 8 months ago

No, I think it should be good, thanks, I just didn't have time to get back to this yet.

shrinktofit commented 7 months ago

I just found soci::row is not-movable too...

olegendo commented 6 months ago

I just found soci::row is not-movable too...

Yes, there are more of those. I think it'd be easier to move on step-by-step after this pending pull request has been merged into master/main.

vadz commented 6 months ago

Sorry for the delay with this one, I'm finally (squash) merging this.

Thanks again for your work on it!

SOCI / soci

make session and connection_parameters movable. #1098