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

[NOSQUASH] Use cpp17, and some cleanups #13587

Merged
merged 8 commits into from Jun 15, 2023
Merged

Conversation

Desour
Copy link
Member

@Desour Desour commented Jun 12, 2023

Descriptions for each commit:

  • Bumps the mimimum gcc and clang versions to the minimum that ubuntu 20.04 supports (gcc 7 and clang 6). And re-enables the CI builds.
  • Bumps the C++ std to 17. (Mostly supported by gcc 7 and clang 6, see https://en.cppreference.com/w/cpp/compiler_support/17.)
  • Fixes a bug where try_lock is called without using the return value. (Found because of [[nodiscard]].)
    Note: The Thread logic still looks faulty to me, i.e. I'm not sure if there's a race condition if the try_lock fails, or if we sometimes call try_lock on a mutex locked by the current thread (which is UB). Someone should fix or explain this at some point.
  • Our Optional type is replaced with std::optional.
  • Forward declarations of the form namespace foo { namespace bar { ... }} are replaced by the new syntax namespace foo::bar { ... }.
  • Our STATIC_ASSERT macro is replaced by static_assert. (This is not C++17 specific, and could've been done much earlier.)
  • The -Wimplicit-fallthrough warnings are no longer disabled. The [[fallthrough]] attribute is now guaranteed to exist. (Btw. we can easily use attributes of newer C++ standards, see also feature testing.)
    (Note: The same could be done for the unused-parameter warnings, with [[maybe_unused]]. But there seem to be many irrlicht functions triggering the warning, and it doesn't seem worth it.)
  • [[noreturn]] is used, as opposed to the compiler-specific syntax. (Also not new in C++17.)

(Related: #13583)

Related (no dependency, but you'll get deprecation warnings without): minetest/irrlicht#202

To do

This PR is a Ready for Review.

How to test

Build.

@Desour Desour added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Jun 12, 2023
@lhofhansl
Copy link
Contributor

Nice. Looks good. The one extra break looks weird, unless that was a (glaring) bug before.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

src/threading/thread.cpp Show resolved Hide resolved
Copy link
Contributor

@numberZero numberZero left a comment

Choose a reason for hiding this comment

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

LGTM

@sfan5 sfan5 merged commit 6a05d63 into minetest:master Jun 15, 2023
15 checks passed
@Desour Desour deleted the use_cpp17 branch June 15, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants