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

Move SuperTux to C++20 #1728

Closed
wants to merge 27 commits into from
Closed

Move SuperTux to C++20 #1728

wants to merge 27 commits into from

Conversation

Grumbel
Copy link
Member

@Grumbel Grumbel commented Apr 19, 2021

This is restoring #1602 back into a compilable state. Let's see what breaks.

@Grumbel
Copy link
Member Author

Grumbel commented Apr 20, 2021

Everything outside of UBPorts compiles with C++20 set. UBPorts is a bit problematic, as it's stuck on an ancient Ubuntu 16.04, which is EOL in a week. No idea how far away the UBPorts 20.04 update is or if there are any PPAs that could give us a newer gcc/clang on that system. UBPorts doesn't support C++17 either.

@Grumbel Grumbel force-pushed the feature/c++20 branch 6 times, most recently from e9f3bfc to 61b46f6 Compare April 20, 2021 17:22
@Grumbel Grumbel marked this pull request as ready for review April 20, 2021 17:56
@Grumbel Grumbel changed the title Move SuperTux to C++17 (or C++20) Move SuperTux to C++20 Apr 20, 2021
@Semphriss
Copy link
Member

Consideration: Ubuntu 18.04 can no longer compile SuperTux out of the box. Neither CMake nor the default GCC, as given by the repositories, support C++20; for the compiler, Canonical provides later versions of Clang; for CMake, I couldn't find later versions that support C++20.

I have no idea how other distros are affected, as I only run Ubuntu 18.04 and 20.04.

@Grumbel Grumbel force-pushed the feature/c++20 branch 2 times, most recently from 4a68838 to 20c451e Compare April 22, 2021 11:12
@Grumbel
Copy link
Member Author

Grumbel commented Apr 22, 2021

I am not too worried about Ubuntu 18.04, as newer compiler are easily available from PPA and we have appimage/flatpak/guix packages on top of that.

But UBPorts will be difficult, as there doesn't seem to be an easy way to get a new compiler onto that. We might need to compile one ourselves.

@Grumbel Grumbel force-pushed the feature/c++20 branch 3 times, most recently from 17abf0d to 1587b36 Compare April 28, 2021 12:49
@Grumbel Grumbel force-pushed the feature/c++20 branch 4 times, most recently from f97713c to aa078ea Compare May 8, 2021 14:54

bool success = true;

// cycle through the directory
for (boost::filesystem::directory_iterator itr(olduserpath); itr != end_itr; ++itr) {
for (std::filesystem::directory_iterator itr(olduserpath); itr != end_itr; ++itr) {
Copy link
Contributor

@jwakely jwakely Aug 9, 2021

Choose a reason for hiding this comment

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

filesystem::directory_iterator can be used directly with range-based for:

Suggested change
for (std::filesystem::directory_iterator itr(olduserpath); itr != end_itr; ++itr) {
for (const auto& file : std::filesystem::directory_iterator(olduserpath)) {

try
{
boost::filesystem::rename(itr->path().string().c_str(), userpath / itr->path().filename());
std::filesystem::rename(itr->path().string().c_str(), userpath / itr->path().filename());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create a temporary string by calling string() and then get a const char* instead of just itr->path.c_str()?

But why get a const char* from it at all, when that just has to be converted to a new filesystem::path (which allocates more memory and parses the path again)?

Suggested change
std::filesystem::rename(itr->path().string().c_str(), userpath / itr->path().filename());
std::filesystem::rename(itr->path(), userpath / itr->path().filename());

@@ -16,6 +16,8 @@

#include "supertux/game_session.hpp"

#include <cfloat>

Copy link
Contributor

Choose a reason for hiding this comment

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

This change is needed independent of any move to C++20, see #1804

@Grumbel Grumbel force-pushed the feature/c++20 branch 2 times, most recently from 8b1fb8c to 69f582c Compare August 9, 2021 21:11
@Grumbel Grumbel force-pushed the feature/c++20 branch 2 times, most recently from 279548c to ca1184d Compare December 22, 2021 10:33
Grumbel added 25 commits July 24, 2022 16:15
The converter takes care of it and the string in question is plain
ASCII.
/home/runner/work/supertux/supertux/src/math/rect.hpp:147:47: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
    return std::tie(left, top, right, bottom) < std::tie(other.left, other.top, other.right, other.bottom);
                                              ^
                                              nullptr
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/tuple:1426:5: note: while rewriting comparison as call to 'operator<=>' declared here
    operator<=>(const tuple<_Tps...>& __t, const tuple<_Ups...>& __u)
This gets fixed in later C++ standards, not worth bothering with.
UBPorts is stuck on Ubuntu 16.04, which provides no compiler capable
of C++20, so this has to wait until they catch up to at least 20.04.
Only update 64bit, leave 32bit on the default gcc, gcc-10 fails:

CMake Error at /usr/local/share/cmake-3.20/Modules/CMakeTestCCompiler.cmake:66 (message):
  The C compiler

    "/usr/bin/gcc"

  is not able to compile a simple test program.
Removing this needs to wait for the commandergenius repository to
remove boost first.
The ..._OTHER variable already contains all the actual flags,
LDFLAGS/CFLAGS variables contain all the flags and include/link
directories combined, which is redundant.
@mrkubax10
Copy link
Member

This is probably replaced by #2473

@mrkubax10
Copy link
Member

Since #2473 was merged a while ago and this PR has a lot of conflicts it was decided to close this PR. Personally I think that it's to early to move to C++20, maybe in 1.5 years when compilers supporting C++20 will be available on more systems.

@mrkubax10 mrkubax10 closed this Jul 27, 2023
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.

4 participants