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

Upgrade to C++20 #7554

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Upgrade to C++20 #7554

wants to merge 12 commits into from

Conversation

messmerd
Copy link
Member

This was a lot easier than I was expecting.

Our Linux and Windows MinGW builds are using GCC/MinGW 9.3 which will likely be the limiting factor here in terms of what we can do with C++20. They barely have any C++20 support.

Some highlights of what we should have now:

  • Designated initializers
  • Class types in non-type template parameters (though Apple Clang may only partially support this)
  • char8_t and std::u8string
  • bit operations and power-of-two operations (albeit with the old names)
  • contains() member function of associative containers

Things we won't have yet:

  • concepts
  • ranges
  • <=>
  • coroutines
  • modules
  • std::span
  • std::format

But once we upgrade to GCC 10, we will have some of these big C++20 features.

Those implicit captures were deprecated in C++20
Unfortunately std::atomic<std::shared_ptr> (P0718R2) is not supported by
GCC until GCC 12 and still is not supported by Clang or Apple Clang, so
it looks like we will continue using std::atomic_load for the time being
@JohannesLorenz
Copy link
Contributor

There are still a few open comments about C++20:

$ git grep '++20'
include/ArrayVector.h:          // TODO C++20: Use std::construct_at
include/ArrayVector.h:  // TODO C++20: Replace with operator<=>
include/ArrayVector.h:  // TODO C++20: Remove
include/Flags.h:        friend constexpr auto operator==(Flags l, Flags r) -> bool { return l.m_value == r.m_value; } // TODO C++20: = default
include/Flags.h:        friend constexpr auto operator!=(Flags l, Flags r) -> bool { return l.m_value != r.m_value; } // TODO C++20: Remove
include/LmmsSemaphore.h:   @note Likely outdated with C++20's std::counting_semaphore
include/lmms_math.h:// @note Once we upgrade to C++20, we could probably use std::formatted_size

Can some of them be solved, or are the compilers limiting us here?

@messmerd
Copy link
Member Author

Yes, we'd need to upgrade our GCC 9.3 (and MinGW 9.3) compilers to at least GCC 10 to begin addressing some of those.

include/ArrayVector.h: // TODO C++20: Use std::construct_at
include/ArrayVector.h: // TODO C++20: Replace with operator<=>
include/ArrayVector.h: // TODO C++20: Remove
include/Flags.h: friend constexpr auto operator==(Flags l, Flags r) -> bool { return l.m_value == r.m_value; } // TODO C++20: = default
include/Flags.h: friend constexpr auto operator!=(Flags l, Flags r) -> bool { return l.m_value != r.m_value; } // TODO C++20: Remove

These will be possible with GCC 10.

include/LmmsSemaphore.h: @note Likely outdated with C++20's std::counting_semaphore

This will be possible with GCC 11.

include/lmms_math.h:// @note Once we upgrade to C++20, we could probably use std::formatted_size

This will be possible with GCC 13.

@JohannesLorenz
Copy link
Contributor

In src/gui/editors, I get this warning with g++ 14.2.1 on Linux:

bitwise operation between different enumeration types 'Qt::Modifier' and 'Qt::Key' is deprecated [-Wdeprecated-enum-enum-conversion]

Files:

AutomationEditor.cpp:2040:45
AutomationEditor.cpp:2044:46
AutomationEditor.cpp:2047:48
AutomationEditor.cpp:2050:48
Editor.cpp:121:46
Editor.cpp:122:46
PianoRoll.cpp:4767:44
PianoRoll.cpp:4768:45
PianoRoll.cpp:4769:46
PianoRoll.cpp:4770:49
PianoRoll.cpp:4829:42
PianoRoll.cpp:4830:43
PianoRoll.cpp:4831:44
PianoRoll.cpp:4852:44
PianoRoll.cpp:4856:45
PianoRoll.cpp:4860:43
PianoRoll.cpp:4864:50

So, this PR adds new warnings on Linux, which might be unwanted, or strategy if you plan to keep them as TODO markers.

@JohannesLorenz
Copy link
Contributor

There might be clang-tidy checks that could be applied now. However, it might be a fair design decision to keep this PR minimal (i.e. without these modernizations), especially since clang-tidy is known to sometimes produce buggy results.

From the clang-tidy list , I see at least:

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

My review is positive, with 1 optional code comment, 2 optional conversation comments (see above).

What I checked:

  • Functional review ✔️
  • Style review ✔️
  • Test review ✔️

cmake/modules/ErrorFlags.cmake Outdated Show resolved Hide resolved
@messmerd
Copy link
Member Author

messmerd commented Nov 7, 2024

In src/gui/editors, I get this warning with g++ 14.2.1 on Linux

Should be fixed now

EDIT: I messed it up - the QKeySequence constructor didn't work the way I expected it to, so I added a helper function to combine keys and modifiers without any deprecation warnings

@JohannesLorenz
Copy link
Contributor

93ed53a (which you just pushed) compiles without warnings for me

AnalyzeTemporaryDtors was deprecated in clang-tidy 16 and fully removed
in clang-tidy 18
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.

3 participants