-
Notifications
You must be signed in to change notification settings - Fork 475
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 SOCI_OVERRIDE macro as conditional C++11 override specifier #596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks, the extra safety of using override
is always nice to have.
@@ -95,6 +95,12 @@ namespace std { | |||
// mode, we just need to check for the minimal compiler version supporting them | |||
// (see https://msdn.microsoft.com/en-us/library/hh567368.aspx). | |||
|
|||
#if defined(SOCI_HAVE_CXX_C11) || (defined(_MSC_VER) && _MSC_VER >= 1800) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, hardly important, but it is actually supposed to work with 1700 (VC11) too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should even, partially or perhaps work with MSVC 1600 as per Support For C++11/14/17 Features.
@@ -135,20 +135,20 @@ struct empty_rowid_backend : details::rowid_backend | |||
{ | |||
empty_rowid_backend(empty_session_backend &session); | |||
|
|||
~empty_rowid_backend(); | |||
~empty_rowid_backend() SOCI_OVERRIDE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I'm personally for using override
on dtors too but this is recommended against by C++ core guidelines, see this closed issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting reading. I've started using the CppCoreGuidelines, but not relying on it strictly due to the performance issues (eg. with gsl::span
).
Meanwhile, I agree with your comment in that thread.
For now, I'll stick SOCI_OVERRIDE
in destructors. After we move to C++11 only and we ever start using the CppCoreGuidelines or discuss/agree to do otherwise. we can always remove it.
I've run |
MSVC++ 1900+ always compile with C++11 mode enabled, so it should be safe to selectively enable 'override'specifier for internal use. It does not enable all C++11 features for SOCI and we still compile with C++11 compilation mode SOCI_CXX_C11=OFF by default. Refactoring performed with clang-tidy-4.0: cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DSOCI_CXX_C11=ON .. run-clang-tidy.py -clang-tidy-binary -header-filter='.*' \ -checks='-*,modernize-use-modernize' -fix
MSVC++ 1900+ always compile with C++11 mode enabled, so it should be safe to selectively enable 'override'specifier for internal use.
It does not enable all C++11 features for SOCI and we still compile with C++11 compilation mode SOCI_CXX_C11=OFF by default.
Refactoring Procedure with clang-tidy
compile_commands.json
) with CMake (sweet, isn't it? :-))clang-tidy
withmodernize-use-override
check on all translations units and actually refactor all the code (-fix
)Here we use run-clang-tidy.py
utility script and run inside the directory with
compile_commands.json
database.The script is recommended way to run
clang-tidy
, it runs concurrent executions, etc,override
and replace withSOCI_OVERRIDE
.Voilà!