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

Require C++14 for building SOCI #996

Merged
merged 18 commits into from
Sep 22, 2022
Merged

Require C++14 for building SOCI #996

merged 18 commits into from
Sep 22, 2022

Conversation

vadz
Copy link
Member

@vadz vadz commented Sep 17, 2022

Update the CI builds and the code as discussed in #907.

@vadz vadz force-pushed the require-cxx14 branch 4 times, most recently from df7c56f to 3beaf1d Compare September 18, 2022 00:33
@vadz vadz changed the title Use more modern compiler versions for AppVeyor CI builds Require C++14 for building SOCI Sep 18, 2022
@vadz vadz force-pushed the require-cxx14 branch 3 times, most recently from 0852c1e to 128beac Compare September 18, 2022 11:50
@vadz vadz mentioned this pull request Sep 18, 2022
2 tasks
Don't prepend CMAKE_CURRENT_SOURCE_DIR to the DSN files, as they may not
necessarily be in the source directory.

It's also arguably more clear to the specify the DSN fully in the tests
makefile instead of modifying it inside soci_backend_test() macro.

No real changes yet.
Allow to override the default MS SQL version value (2014) by setting
MSSQL_VER environment variable -- this will be used to test SOCI with
different versions of MS SQL in the CI builds soon.
Otherwise they're just going to fail anyhow.
Drop MSVS < 2015 versions and add 2019 and 2022 and use (the already
available on AppVeyor) gcc 8.1 instead of (manually installed) 4.8.
Don't set CMAKE_CXX_STANDARD in the environment, it will be set in CMake
makefiles themselves soon.
Remove checks for C++11 but keep SOCI_OVERRIDE, SOCI_NOEXCEPT etc macros
for now, they will be cleaned up later.
Remove SOCI_CXX11 option which doesn't make sense any longer.
No more need for SOCI_STATIC_ASSERT.
Stop using soci::cxx_details::auto_ptr<>.

Also use unique_ptr<> for session::query_transformation_ instead of
managing its lifetime manually, this is shorter and less error-prone.
This results in build problems with clang 15 and libc++ (see SOCI#984) and
also with MSVC due to the use of min/max function that are defined as
macros.

This is just a temporary workaround, the real solution will be to update
our CATCH version.
We don't need the macro any longer now that we use C++14.
Similarly to the previous commit, remove the macro which is not needed
any more now that we require C++14.
include/soci/once-temp-type.h Show resolved Hide resolved
include/soci/rowset.h Show resolved Hide resolved
include/soci/session.h Show resolved Hide resolved
include/soci/soci-platform.h Show resolved Hide resolved
@@ -73,15 +73,15 @@ class standard_logger_impl : public logger_impl
} // namespace anonymous

session::session()
: once(this), prepare(this), query_transformation_(NULL),
: once(this), prepare(this),
logger_(new standard_logger_impl),
uppercaseColumnNames_(false), backEnd_(NULL),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While transitioning to a modern cpp standard, it would probably also be good to replace all NULL with nullptr

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm going to use clang-tidy to do this later, but let's merge this one first.

@vadz vadz marked this pull request as ready for review September 18, 2022 18:08
@vadz
Copy link
Member Author

vadz commented Sep 18, 2022

I think this can be merged now. There are certainly more things to do, e.g. using nullptr mentioned above and more, and I'll use clang-tidy modernization checks to change this later, but this already will allow any future PRs to not have to care about C++98 compatibility, and the sooner this can happen the better.

Sorry if this results in any conflicts with the still open PRs, but hopefully they won't be difficult to resolve.

If anybody has any objections to merging this, please comment here soon. Thanks!

@vadz vadz merged commit 72ac2e2 into SOCI:master Sep 22, 2022
@vadz vadz deleted the require-cxx14 branch September 22, 2022 14:14
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