Skip to content

Conversation

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Dec 9, 2020

Just to prove that it does not work with our CI.

@JohannesLorenz
Copy link
Contributor Author

Quoting @DomClark 's response from Discord:

Regarding C++17: I really don't think it will take us three years to fix a few compiler errors.

  1. Linux builds are failing because AppImage requires the oldest version of Ubuntu still in LTS, and 16.04 doesn't have a C++17 compiler. We can upgrade to 18.04 in May, which comes with GCC 7 and C++17 support.
  2. MinGW and macOS builds are failing because the caps plugins use the register storage class, which is deprecated since C++11 and removed in C++17. Fixing this only requires removing the register keyword, which we should do anyway since it's deprecated.
  3. MSVC builds are failing because the new std::byte type is causing ambiguities inside the Windows API headers. This is because we have using namespace std; in lmms_math.h, which is a really bad idea in general and should be removed anyway. This should only require prepending std:: to some types and mathematical functions.

@JohannesLorenz
Copy link
Contributor Author

IMO all mentioned points should be handled in separate PRs. 2 and 3 can be done now, 1 must wait until May.

-> This PR will remain draft until May 2021.

@JohannesLorenz
Copy link
Contributor Author

@irrenhaus3 Did you start fixing any of the 3 blockers above already? If yes, can you reference the PRs?

@irrenhaus3
Copy link
Contributor

@irrenhaus3 Did you start fixing any of the 3 blockers above already? If yes, can you reference the PRs?

Blocker 3 (using namespace std; in LMMS headers) should be resolved via #6076

@DomClark
Copy link
Member

(2) is now resolved by #6133. May has come and gone, so it is possible to resolve (1) as well now.

@DomClark
Copy link
Member

Additional lines to change are:

set(CMAKE_CXX_STANDARD 11)

SET(CMAKE_REQUIRED_FLAGS "-std=c++14")

SET(CMAKE_CXX_STANDARD 14)

Also, include/stdshims.h is no longer required for now.

@JohannesLorenz
Copy link
Contributor Author

Additional lines to change are:

set(CMAKE_CXX_STANDARD 11)

SET(CMAKE_REQUIRED_FLAGS "-std=c++14")

SET(CMAKE_CXX_STANDARD 14)

Also, include/stdshims.h is no longer required for now.

Thanks. Reworked it all, force-pushed and rebased to current master.

Result: Only CircleCi's linux.gcc is not compiling, due to the too old Ubuntu version (which @PhysSong plans to update soon).

@DomClark
Copy link
Member

I don't think the set(CMAKE_CXX_STANDARD ...) at the top of plugins/vst_base/RemoteVstPlugin/CMakeLists.txt is redundant - the directory is (usually) built using ExternalProject_Add, so doesn't inherit CMAKE_CXX_STANDARD from plugins/CMakeLists.txt. Also, I still think tests/CMakeLists.txt should be changed to use C++17 - if we start using C++17 features in the headers, we still want the tests to build successfully.

@JohannesLorenz
Copy link
Contributor Author

Thanks again Dom, all fixed and pushed.

This replaces `set(CMAKE_CXX_STANDARD 14)` by `set(CMAKE_CXX_STANDARD 17)`
wherever it is required.

Additionally:

* raise `CMAKE_MINIMUM_REQUIRED(VERSION ...)` to `3.8` (the minimum
  that supports C++17)
* `stdshims.h` is now unused and thus removed
@JohannesLorenz
Copy link
Contributor Author

Rebased on master commit which uses the 18.04 docker container. No further changes made after Dom's review.

Build passed (AppVeyor failed now due to problems on master, but passed previously, the rebase should not have affected it).

Will merge in 3 days if there are no more complaints.

@PhysSong PhysSong marked this pull request as ready for review September 12, 2021 02:06
@JohannesLorenz JohannesLorenz merged commit 8a9a2fa into LMMS:master Sep 17, 2021
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