-
-
Notifications
You must be signed in to change notification settings - Fork 23.5k
Core: Bump C++ Standard to C++20 #100749
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
base: master
Are you sure you want to change the base?
Core: Bump C++ Standard to C++20 #100749
Conversation
|
Has the concerns with support for c++20 on niche compilers like those for various console ports been resolved? That has been the main thing holding this back other than just the workload, we really don't want various porting providers running afoul of this |
|
I'm not certain if that's been resolved, and I certainly wouldn't want this merged before that was the case either. |
|
Absolutely, generally absolutely in favor of adding various new QOL features from C++20, brought it up in the platforms channel and we'll see what information we can dig up |
494f436 to
078c644
Compare
078c644 to
257e648
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
257e648 to
a758194
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a758194 to
fa4450f
Compare
|
I want to throw my support behind the concept of moving to c++20, in particular it will give us access to: https://en.cppreference.com/w/cpp/utility/source_location Which will allow us to instrument our builds better, allowing us to log where our error macros happen. So instead of getting a message that p_index is out of range in cowdata, we could potentially log where the actual call that went out of index happened instead. It'll take a bit of doing, but I think that that alone is worth the upgrade. |
|
Oh wow, that looks extremely useful! The header looks pretty lightweight, similar to |
Maybe we should aim low for this initially then. Do your first 2 commits as separate PRs, at that point we technically have a C++20 codebase, we can then start using I think there's things in this still that are more controversial than just the switch to a newer standard. |
|
After some local testing, it'd definitely be more involved than this PR should reasonably change. While using it as a drop-in replacement for |
That's what I'm already doing tbh. Everything else I've tried to dig into has felt like it goes beyond the scope of drop-in QOL bumps, so the changes remained conservative (albeit wide-reaching). I could maybe see myself reverting the three-way comparison commit if people are THAT skeptical (even if its inclusion is an inevitability), but none of the others strike me as intrusive or contentious. |
Oh yeah, it is. I did say "It'll take some doing", but because it is a type it can be put in template parameters, so we can schlep it around to the macro site. I wasn't suggesting we do it in this PR, I just think having C++20 is a prerequisite to starting doing it! |
|
As im currently working on refactoring Variant, i feel like concepts from C++20 would be extremely helpful allowing us to more easily register them and remove clutter. I'm imagining a system that would only require you to modify the class itself and everything else would handle itself. |
d6c9b7a to
382f1e4
Compare
2147cf4 to
35c4717
Compare
284ab5e to
44d5db8
Compare
182875b to
e3dba5b
Compare
e3dba5b to
2e646ad
Compare
f4d3b99 to
4c3144a
Compare
|
Revisited the structure after the core meeting, and managed to significantly decrease the number of changed files/lines. To ensure C++17 compatibility, there's no more removed operators; only additional operators to account for the ambiguous cases in C++20.
I don't believe that there's any real way to circumvent the Variant operator bloat, as it otherwise requires a VERY invasive number of changes. As the comment notes, it's an area that can be revisited with a more general pass later. |
dsnopek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! There's nothing that seems particularly dangerous or problematic for the areas of the engine I usually work in
Ivorforce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate that you got it even more minimal!
16cde11 to
3e6d375
Compare
raulsntos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried compiling this branch and I'm getting these warnings:
modules/mono/csharp_script.cpp:1110:59: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'GCHandleIntPtr' and 'GCHandleIntPtr') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
if (!r_gchandle.is_released() && r_gchandle.get_intptr() == p_gchandle_to_free) { // Do not lock unnecessarily
~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
modules/mono/mono_gd/../mono_gc_handle.h:48:22: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
_FORCE_INLINE_ bool operator==(const GCHandleIntPtr &p_other) { return value == p_other.value; }
^
modules/mono/csharp_script.cpp:1112:60: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'GCHandleIntPtr' and 'GCHandleIntPtr') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
if (!r_gchandle.is_released() && r_gchandle.get_intptr() == p_gchandle_to_free) {
~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
modules/mono/mono_gd/../mono_gc_handle.h:48:22: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
_FORCE_INLINE_ bool operator==(const GCHandleIntPtr &p_other) { return value == p_other.value; }
^
modules/mono/csharp_script.cpp:1120:55: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'GCHandleIntPtr' and 'GCHandleIntPtr') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
if (!gchandle.is_released() && gchandle.get_intptr() == p_gchandle_to_free) { // Do not lock unnecessarily
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
modules/mono/mono_gd/../mono_gc_handle.h:48:22: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
_FORCE_INLINE_ bool operator==(const GCHandleIntPtr &p_other) { return value == p_other.value; }
^
modules/mono/csharp_script.cpp:1122:56: warning: ISO C++20 considers use of overloaded operator '==' (with operand types 'GCHandleIntPtr' and 'GCHandleIntPtr') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
if (!gchandle.is_released() && gchandle.get_intptr() == p_gchandle_to_free) {
~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~
modules/mono/mono_gd/../mono_gc_handle.h:48:22: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
_FORCE_INLINE_ bool operator==(const GCHandleIntPtr &p_other) { return value == p_other.value; }
^
However, it finishes building and everything appears to work as expected.
3e6d375 to
aedd081
Compare
aedd081 to
0b36e2d
Compare
Interesting that didn't cause warnings on our CI. Still, that's probably caused by the equality operators in that class not being |
raulsntos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not getting any warnings now, and I see no problems with this for the .NET area.
Calinou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally on Windows 11 24H2 with MSVC 2022, it works as expected. I've also tested an Ubuntu 22.04 Docker container and Godot can still be built.
Note that this makes it no longer possible to compile Godot on Ubuntu 20.04 (unless you install a more recent GCC/Clang through third-party repositories), but 20.04 is EOL since April 2025 anyway.
Code looks good to me.
Some notes:
- Is MSVC 2019 tested (on its latest update) to make sure it works? I don't have it installed, but I don't remember if it has full C++20 support.
- Using Apple's Clang, is it still possible to compile on old macOS versions like 10.15 or 11? We still support 10.15 currently, although only with the Compatibility renderer. It's not a big deal if it can't be compiled on 10.15 anymore, as long as binaries compiled on newer macOS versions still work there.
In contrast to the above PR, which attempted to simultaneously support C++17 alongside later standards, this PR aims to make our baseline C++20 outright. The immediate benefit is removing the need for several workarounds via macros &
#if __cpluspluswrappers, making for a much cleaner codebase overall. The much, much bigger benefit is the freedom to actually integrate C++20 features, allowing this PR to satisfy our contribution Best Practices & not exist as a solution looking for a problem.EDIT: After a core meeting, it was instead decided that integrations of features would be best handled in entirely separate PRs, so that we can pick-and-choose which features are best to integrate. While that dampens the effect of this PR in isolation, it's now doing so with the knowledge that it will not exist in isolation and would soon be followed with proper integrations of the new features that the repo would directly benefit from. Future edits of this OP will point to specific discussions/implementations of said features up until this is merged.