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

Merged
merged 15 commits into from
Nov 23, 2024
Merged

Upgrade to C++20 #7554

merged 15 commits into from
Nov 23, 2024

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented Oct 22, 2024

This was a lot easier than I was expecting.

Our Linux and Windows MinGW builds are using GCC/MinGW 9.3 which will be the limiting factor here in terms of what we can do with C++20. They barely have any C++20 support, but even the small amount they do provide should be useful.

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

@messmerd messmerd requested a review from DomClark November 10, 2024 20:49
AnalyzeTemporaryDtors was deprecated in clang-tidy 16 and fully removed
in clang-tidy 18
@JohannesLorenz
Copy link
Contributor

This has 2 reviews of developers, and all kinds of reviews are covered. No one else complained in the last 2 weeks. Is there any reason to prevent merging?

Prevents issues if any enum flags in `args` have bits in common
@JohannesLorenz
Copy link
Contributor

After the rework, I now tested the key sequences - still working.

@messmerd messmerd merged commit c21a7cd into LMMS:master Nov 23, 2024
10 of 11 checks passed
@messmerd messmerd deleted the cpp20 branch November 23, 2024 04:47
rubiefawn pushed a commit to rubiefawn/lmms that referenced this pull request Nov 28, 2024
* Compile in C++20 mode

* Fix implicit lambda captures of `this` by `[=]`

Those implicit captures were deprecated in C++20

* Silence MSVC atomic std::shared_ptr warning

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

* Use C++20 in RemoteVstPlugin

* Simplification

* Add comment

* Fix bitwise operations between different enumeration types

* Revert "Fix bitwise operations between different enumeration types"

This reverts commit d45792c.

* Use a helper function to combine keys and modifiers

* Remove AnalyzeTemporaryDtors from .clang-tidy

AnalyzeTemporaryDtors was deprecated in clang-tidy 16 and fully removed
in clang-tidy 18

* Use C++20 in .clang-format

* Use bitwise OR

Prevents issues if any enum flags in `args` have bits in common
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