godot icon indicating copy to clipboard operation
godot copied to clipboard

Core: Support c++20 compilation

Open Repiteo opened this issue 1 year ago • 3 comments
trafficstars

Adds a new cpp_standard experimental variable to SCons, to allow the user to choose which C++ standard to compile with. The choices are "17" and "20", defaulting to 17. In addition, this makes changes across the repo to allow compilation of c++20 to be possible in the first place, which primarily comes down to more operators to prevent ambiguous conversions. Currently, a c++20 build successfully compiles (with werror=no).

~~Submitted as a draft, as while the equality operators are accounted for, there's still the ordering operators. Inequality operators could be handled with a wrapper, but ordering operators will be a bit more involved than that; ideally the <=> operator will be used, so long as parity with the secondary operators is kept between 17 & 20. There's also the matter of making sure the other warnings are accounted for, which are largely secondary but still important so long as they don't negatively impact c++17 (which will remain the repo "default").~~ EDIT: All warnings accounted for, no longer a draft! Ordering operators are outside the scope of this PR, now that I'm more familiar with just how much would need to change.

EDIT: Added "support" for c++23 as well, albeit to a much lesser extent because its currently experimental.

Repiteo avatar Mar 18 '24 20:03 Repiteo

I have no opinion on adding c++ 20 support. It depends on what the others think.

fire avatar Aug 19 '24 20:08 fire

Hey, I noticed we are using a special #define to check for c++20 support.

Have we considered using the __cplusplus macro to gate on language feature support?

for c++20, it should be sufficient to use this check instead: #if __cplusplus >= 202002L See https://stackoverflow.com/a/53557910

lyuma avatar Aug 19 '24 21:08 lyuma

The lack of out-of-the-box MSVC support made me hesitant, but seeing as there's a compiler option to enable it I might revisit the idea. Will have to test if the option in isolation causes any issues first

Repiteo avatar Aug 28 '24 20:08 Repiteo

I have no opinion on adding c++ 20 support. It depends on what the others think.

I think it's nice if someday we are choosing to bump up our standards to C++20. It will compile off-the-bat.

adamscott avatar Nov 05 '24 18:11 adamscott

It's possible that the C++20 minimum versions can be more lax than I originally expected, at least for our purposes, so I'm trying that out this rebase. Ideally, C++20 will be fully-functional on ubuntu-22.04

EDIT: Yep, seems good! Should make an eventual transition to C++20 much more reasonable

Repiteo avatar Nov 26 '24 16:11 Repiteo

Should make an eventual transition to C++20 much more reasonable

Especially if we want to be able to use consteval and constinit. Could happen sooner than we think.

adamscott avatar Dec 04 '24 15:12 adamscott

I'm not sure I like the new INEQUALITY_OPERATOR macro, we don't have those for other operators. It feels out of place and a little confusing.

It kind of smells like a workaround for something.

hpvb avatar Dec 13 '24 00:12 hpvb

The alternative is adding a bunch of cpp20 conditionals everywhere. It wouldn't be dealbreaking, but I wasn't sure if that was desired when starting off

Repiteo avatar Dec 13 '24 02:12 Repiteo

The alternative is adding a bunch of cpp20 conditionals everywhere. It wouldn't be dealbreaking, but I wasn't sure if that was desired when starting off

I think we should just either move to c++20, or not. I don't think that supporting c++17 while also using c++20 features is going to lead to a very pleasant codebase personally.

hpvb avatar Dec 13 '24 15:12 hpvb

I think we should just either move to c++20, or not. I don't think that supporting c++17 while also using c++20 features is going to lead to a very pleasant codebase personally.

To clarify, this isn't using C++20 features. This is only introducing fixes to make C++20 compilation work outright. Actually adding c++20 functionality wasn't within the original scope of the PR.

I would love to migrate to C++20 fully. Concepts in particular would be the biggest draw, but consteval & three-way comparison aren't far behind.

Repiteo avatar Dec 13 '24 16:12 Repiteo

To clarify, this isn't using C++20 features. This is only introducing fixes to make C++20 compilation work outright. Actually adding c++20 functionality wasn't within the original scope of the PR.

I'll have to have a deeper look at that macro then, surely there's a way to do this is a way that's less jarring!

Thanks for working on this by the way, I don't mean to sound super negative.

hpvb avatar Dec 13 '24 19:12 hpvb

The only two real alternative is giving all inequality operators #if __cplusplus < 202002L wrappers, because C++20 fundamentally changes how equality/comparison operators are parsed. As a whole, the C++20 changes are a net positive, as it means a lot of logic can be defaulted away^1 and don't require multi-scope shenanigans:

class String {
...
	bool operator==(const String &p_str) const; // Universal in C++17 & C++20
	bool operator==(const char *p_str) const; // One-way in C++17, universal in C++20
...
}
// bool operator==(const String &p_str, const String &p_str); // "Ambiguous" error in C++17 & C++20
bool operator==(const char *p_cstr, const String &p_str); // Workaround in C++17, "ambiguous" error in C++20

The latter bit has to do with C++20 automatically rearranging the == (and <=>) operators in ways that previously required explicit specification^2. So now a == b now implicitly defines b == a, a != b, and b != a. However, part of this automatic setup means that the previously explicit operators can cause ambiguity errors, which is why the majority of explicitly defined comparison operators are being deprecated in the standard library — <=> (and == if non-default) is all you'd need! If I were to no longer utilize the INEQUALITY_OPERATOR macro, instead incorporating the new <compare> library with C++20 code, this would result in:

// RID, defaulted
#if __cplusplus < 202002L
	_ALWAYS_INLINE_ bool operator==(const RID &p_rid) const {
		return _id == p_rid._id;
	}
	_ALWAYS_INLINE_ bool operator<(const RID &p_rid) const {
		return _id < p_rid._id;
	}
	_ALWAYS_INLINE_ bool operator<=(const RID &p_rid) const {
		return _id <= p_rid._id;
	}
	_ALWAYS_INLINE_ bool operator>(const RID &p_rid) const {
		return _id > p_rid._id;
	}
	_ALWAYS_INLINE_ bool operator>=(const RID &p_rid) const {
		return _id >= p_rid._id;
	}
	_ALWAYS_INLINE_ bool operator!=(const RID &p_rid) const {
		return _id != p_rid._id;
	}
#else
	_ALWAYS_INLINE_ std::strong_ordering operator<=>(const RID &p_rid) const = default; // Handles literally everything.
#endif

// Vector2, non-default
#if __cplusplus < 202002L
	bool operator==(const Vector2 &p_other) const;
	bool operator!=(const Vector2 &p_other) const;

	bool operator<(const Vector2 &p_other) const { return x == p_other.x ? (y < p_other.y) : (x < p_other.x); }
	bool operator>(const Vector2 &p_other) const { return x == p_other.x ? (y > p_other.y) : (x > p_other.x); }
	bool operator<=(const Vector2 &p_other) const { return x == p_other.x ? (y <= p_other.y) : (x < p_other.x); }
	bool operator>=(const Vector2 &p_other) const { return x == p_other.x ? (y >= p_other.y) : (x > p_other.x); }
#else
	std::weak_ordering operator<=>(const Vector2 &p_other) const { // Non-default thanks to anonymous unions.
		if (x <=> p_other.x != 0) {
			return x <=> p_other.x;
		}
		return y <=> p_other.y;
	}
	bool operator==(const Vector2 &p_other) const { return *this <=> p_other == 0; } // Required because non-default.

	// Explicit comparison operators *between* types often needed in C++20, so they can be scoped.
	std::weak_ordering operator<=>(const Vector2i &p_other) const; 
	bool operator==(const Vector2i &p_other) const;
#endif

Repiteo avatar Dec 14 '24 17:12 Repiteo

Well, I'm not the final arbiter of this decision. But I think my preference would go to a PR that just sets our minimum compiler to c++20, and makes these changes. I'd support that, I don't think there's any pressing reason at this time to support older compilers. Especially since we have the Godot toolchains for people who want to build on ancient Linux distros.

Again, not my choice, but I am really grossed out by the macro approach, even though I agree with you that it is the "least bad" option.

hpvb avatar Dec 16 '24 17:12 hpvb

I'm all aboard the C++20 train. Maybe for 4.5?

adamscott avatar Dec 17 '24 19:12 adamscott

Superseded by #100749

Repiteo avatar Mar 09 '25 15:03 Repiteo