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

Switch to C++20 #976

Merged
merged 13 commits into from
Nov 20, 2024
Merged

Switch to C++20 #976

merged 13 commits into from
Nov 20, 2024

Conversation

kring
Copy link
Member

@kring kring commented Nov 5, 2024

The main motivation here is support for the upcoming Unreal Engine 5.5. It uses the GSL library, as cesium-native does, but it's a modified (broken) version of it, so we can't use UE's version in cesium-native. Using our own copy creates conflicts with Unreal's. So one possible solution is to switch to C++20, change all the gsl::span to std::span, and drop the dependency on GSL. This PR is the first step in that.

Some notes:

  1. The version of Async++ that we were previously using, via vcpkg, does not work in C++20. A new version that fixes this was only tagged very recently, and is not yet in a tagged vcpkg release. Which means that this branch uses a prerelease vcpkg for the moment. We should probably wait until the next vcpkg release, and then use that, before we merge this. I think it should be really soon.
  2. Compiling TestPropertyTableView.cpp for C++20 RelWithDebInfo on Ubuntu 22.04 uses a ton of memory, crashing the GitHub Actions runner. We should be able to fix this by moving away from std::variant, as we've done in Unreal and Unity, now that we're on C++20. But for now I've just added a build step to increase available swap space so that the build can complete successfully.
  3. C++20 made the u8string method of std::filesystem::path returning a std::u8string instead of a regular std::string. This is fairly painful because we were already using std::string to represent a UTF-8 string (in C++17 there was no better alternative), and there's apparently no conversion between u8string and string. We should consider whether we should standardize on u8string everywhere, but I'm not sure of the implications. For now I've done the simplest thing: I've added a conversion and called it where we need it.
  4. Our generated Spec classes had a private constructor, when it should have been protected. C++17 didn't care (surprisingly!), but C++20 does. C++20 also requires that derived classes explicitly declare a public constructor if they're expected to have one.
  5. I reworked how we interface with STB Image Resize, because slamming it in a namespace is a bit dodgy and was creating compiler errors (not sure if it was a result of C++20 or a result of the changed vcpkg version). Now we just mark it static and expose helper functions that can be used to call it from outside the one module where it's defined.

@kring
Copy link
Member Author

kring commented Nov 12, 2024

I'm marking this ready to review. My only small reservation is that it still uses (by necessity) a pre-release vcpkg repo. I think the new version, including the fix we require, should be out by the end of the month.

@kring kring marked this pull request as ready for review November 12, 2024 00:28
@kring kring added this to the December 2024 Release milestone Nov 14, 2024
@j9liu j9liu self-requested a review November 18, 2024 21:10
Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

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

@kring looks good to me! I just had one comment that I wanted to run by you before merging

* @param s The `std::u8string`.
* @return The equivalent `std::string`.
*/
static std::string toStringUtf8(const std::u8string& s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick but, I feel it would be more informative to say toStdString? The input and output are both UTF8, and the only thing really changing is putting it in a std::string wrapper.

Suggested change
static std::string toStringUtf8(const std::u8string& s);
static std::string toStdString(const std::u8string& s);

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the Utf8 bit is important. It's true the function doesn't change the UTF8-ness of the string, but that's the key feature to communicate. String was meant to imply std::string, but maybe that's not obvious. How about toStdStringUtf8?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fair point. I was thinking back to Unreal / Unity contexts where there are multiple string types, so they have functions like ToFString(), toStlString(), etc. But specifying std::string is probably overkill for Native, where C++ std::string is the norm. You can disregard this comment!

@kring
Copy link
Member Author

kring commented Nov 19, 2024

New release of vcpkg is out, which should let us move off the prerelease one used in this branch. I'll do that today.
https://github.com/microsoft/vcpkg/releases/tag/2024.11.16

@j9liu
Copy link
Contributor

j9liu commented Nov 20, 2024

@kring I went ahead and switched us over to 2024.11.16. Looks like I haven't broken CI, so I'll merge now. Thanks! 😄

@j9liu j9liu merged commit 73801b5 into main Nov 20, 2024
20 checks passed
@j9liu j9liu deleted the cpp20 branch November 20, 2024 18:41
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.

2 participants