SOCI / soci

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

Postgresql backend does not compile for C++17 onwards with -Werror #754

Open 5cript opened 4 years ago

5cript commented 4 years ago

In the postgresql/statement.cpp are 2 functions that might not be used ever. This is "just" a warning, but since there is a -Werror somewhere, they become errors. Since these two are the only two errors I had, even though I compiled all of soci and soci-postgresql in C++17 just fine, I post this issue here.

https://github.com/SOCI/soci/blob/c2f934aa95a283f2a02ab7087d9000330f3fc72b/src/backends/postgresql/statement.cpp#L27 and https://github.com/SOCI/soci/blob/c2f934aa95a283f2a02ab7087d9000330f3fc72b/src/backends/postgresql/statement.cpp#L44

The above is fixable by checking against

// note that it is okay to not check _MSC_VER, because if __cplusplus is not defined in older Visual Studio versions, its also not suporting C++17.
#if (__cplusplus >= 201704L)
[[maybe_unused]] void wait_until_operation_complete(postgresql_session_backend & session)
#else
void wait_until_operation_complete(postgresql_session_backend & session)
#endif

The below is fixable like:

#if !defined(SOCI_POSTGRESQL_NOSINGLEROWMODE) && (__cplusplus >= 201704L)
[[maybe_unused]] void throw_soci_error(PGconn * conn, const char * msg)
#else
void throw_soci_error(PGconn * conn, const char * msg)
#endif

Please let me know if you have (strong) objections and why. I haven't tried C++14 on g++ yet. This quick fix (albeit modern) i applied might leave a gap. I dont even know why the warning does not appear on C++98?

edit: probably because warning 'unused xyz' more aggressively got viable when the attribute was introduced.

mloskot commented 4 years ago

I dont even know why the warning does not appear on C++98?

No idea.

Please let me know if you have (strong) objections and why.

To me your proposal seems not affecting C++98 users or users of any version that's not 201704L, so I have no objections.

I just have a question: the title of your issue says "modern C++ version" (would be helpful if it was more specific), then your change is for >=201704L. Do you consider "modern C++ version" here as C++2a/20 and later? Or, there is a mistake and 201704L should read 201703L (i.e. C++17 or later)?


Feel free to submit PR.

5cript commented 4 years ago

I meant C++17 onwards, so I botched the value? No matter, would have noticed in PR. I will make a PR as soon as I can, when I tested it with all different C++ versions.

mloskot commented 4 years ago

I meant C++17 onwards, so I botched the value?

It seems so, given all the below agree that C++17 is 201703L.

https://en.cppreference.com/w/cpp/preprocessor/replace https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ https://docs.microsoft.com/en-us/cpp/build/reference/zc-cplusplus