Should bitfields types be unsigned integers instead of signed integers?

[TCROC]

Tested versions

Pop OS 22.04: Godot Engine v4.3.dev.mono.custom_build.bbb3531fa
Windows 10: Godot Engine v4.3.dev.mono.custom_build.bbb3531fa

System information

Pop OS 22.04, Windows 10

Issue description

We found that Windows was actually treating 0xffffffff bitflags as a signed integer with a value of -1 when compared to Linux where it was treated as unsigned and assigned a value of 4294967295.

This resulted in Gdext panicing when generating Godot bindings as seen in this PR for the fix over here: godot-rust/gdext#627

Godot currently uses signed integers when binding all of its constants including bitflags:

godot/core/object/class_db.cpp

Line 949 in 81f3d43

void ClassDB::bind_integer_constant(const StringName &p_class, const StringName &p_enum, const StringName &p_name, int64_t p_constant, bool p_is_bitfield) {

So we are seeking further information and discussion around the data type that Godot should enforce for bitflags. Should it stay with the currently signed value? Are there instances where negative values make sense for bitflags? Is there a reason Steam's sdk evaluates to this negative value on Windows and is this expected behavior? If Godot changed to unsigned, would it still be compatible with Steam?

I don't expect all of these questions to necessarily be answered by the Godot team, but I wanted to get the discussion started while we also change the datatype on the gdext side to match Godot's datatype.

Here are the current issues / prs for reference:

godotsteam: GodotSteam/GodotSteam#424
gdext: godot-rust/gdext#627

Steps to reproduce

Build godotsteam from here: https://github.com/CoaguCo-Industries/GodotSteam/tree/godot4.

Evaluate the bitflags as describe here: GodotSteam/GodotSteam#424

See that the constant on Linux is 4294967295

And the constant on Windows is -1

Minimal reproduction project (MRP)

https://github.com/CoaguCo-Industries/GodotSteam/tree/godot4

[lawnjelly]

It has come up before, see e.g. #61757 , #88424 (comment) .

[TCROC]

Thanks for those! So I guess the next discussion to be had is if we should extend that enforcement of unsigned integers to the class db level for bitflags:

godot/core/object/class_db.cpp

Line 949 in 81f3d43

void ClassDB::bind_integer_constant(const StringName &p_class, const StringName &p_enum, const StringName &p_name, int64_t p_constant, bool p_is_bitfield) {

What impact that may have on sdks like Steam? Will it be compatible still and / or is there an expected reason it is negative on Windows?

[Bromeon]

To bring this to a discussion about concrete actionables -- we currently have this in the GDExtension C header:

typedef int64_t GDExtensionInt;

typedef void (*GDExtensionInterfaceClassdbRegisterExtensionClassIntegerConstant)(
    GDExtensionClassLibraryPtr p_library,
    GDExtensionConstStringNamePtr p_class_name,
    GDExtensionConstStringNamePtr p_enum_name,
    GDExtensionConstStringNamePtr p_constant_name,
    GDExtensionInt p_constant_value,
    GDExtensionBool p_is_bitfield
);

The signed int64_t is causing some confusion. From the PRs linked by @lawnjelly, it seems like there's a tendency to move towards unsigned numbers for bitfields, at least in the implementation.

---

I see two options:

1. Add a new C function with the correct type

// only for bitfields, not enums
typedef void (*GDExtensionInterfaceClassdbRegisterExtensionClassBitfieldConstant)(
    GDExtensionClassLibraryPtr p_library,
    GDExtensionConstStringNamePtr p_class_name,
    GDExtensionConstStringNamePtr p_enum_name,
    GDExtensionConstStringNamePtr p_constant_name,
    uint64_t p_constant_value, // now unsigned
    // no p_is_bitfield value
);

We can either keep the old ClassdbRegisterExtensionClassIntegerConstant method.
Or deprecate it and add a ClassdbRegisterExtensionClassEnumConstant one without p_is_bitfield parameter.

2. Keep signature, improve docs

If you think moving away from the existing signature is too much of a change, we could go for this approach.

Clearly specify that p_constant_value, albeit signed, is interpreted as unsigned if p_is_bitfield is set to true.
Encourage bindings to use uint64_t to represent that type on their side.

[Naros]

Do you have any idea the extent of the impact a change like this could have, @Bromeon?

At least at my day job, we typically err on the side of a combination of both of these, starting with (2) and at some point introducing the new API in conjunction with the deprecation of the old toward the EoL of the current major just before moving to the next major when there is a large impact.

[Bromeon]

The existing API would keep working; GDExtension generally provides backwards compatibility down to Godot 4.1. So no one would be forced to update.

Migration would likely be quite straightforward, too: since the underlying value is only a bit pattern and has no numeric meaning, one can simply use uint64_t instead of int64_t when registering a constant (or cast when necessary, although I don't see why a bitfield should be signed in the first place).

[HuntJSparra]

I'm in favor the new C function. I've seen a few libraries that use -1 as shorthand for "all flags on." It's very convenient, but you can run into trouble when casting from a smaller unsigned integer to a larger signed one because -1 is a different set of bits depending on your integer size. Enums that have bitfields (e.g., Steamworks API) are extra fun because the default type for enums can depend on the compiler (see #61757). Unsigned bitfields prevent this.

For the new API, I have a few questions:

1. Would the new C function be called via a new ClassDB method `bind_bitfield_constant``?
2. Binding unsigned integers are useful even outside of bitfields (e.g., IDs). Is there any chance this could be a generic bind_unsigned_integer_constant API? It out of the scope of this issue, but I wanted to bring it up now because I can see people misusing the new bitfield API for that purpose. I'd do that.

[Bromeon]

1. Would the new C function be called via a new ClassDB method bind_bitfield_constant?

Very good question. I originally intended to use the existing ClassDB::bind_integer_constant(). But maybe this is too short-sighted -- when querying constants, e.g. via ClassDB::class_get_integer_constant() API, you could then get a different value (because signed) than registered.

Maybe this is something to accept, I'm not sure if promoting bitfields to their own independent "domain" would be desired, and it would also be problematic for backwards compatibility. Or the existing method only returns constants registered the old way, and bitfields (with is_bitfield=true) would have their own getter. That however means we'd need yet another flag for old vs. new bitfields...

Or it's creating too much complexity 🤔 my intention was mostly to make clear to GDExtension developers that bitfields are really just bits, and the signedness is irrelevant (thus unsigned, because more common). I'm not sure if we should rework the whole way how constants are handled internally in Godot.

---

2. Binding unsigned integers are useful even outside of bitfields (e.g., IDs). Is there any chance this could be a generic bind_unsigned_integer_constant API? It out of the scope of this issue, but I wanted to bring it up now because I can see people misusing the new bitfield API for that purpose. I'd do that.

One problem I see is that the most common Godot language, GDScript, does not support unsigned integers. So if you registered 0xFFFFFFFF as unsigned, it would have a different value in GDScript. Godot itself doesn't differentiate between signed and unsigned constants, so this information would be lost. At that point, it's probably better to stick to the supported int64_t.

While you could abuse the bitfield registration to pass in uint64_t constant, it would a) be marked as is_bitfield=true (which is probably not what you want) and b) internally it would still be stored as int64_t. But as mentioned above, this would depend on whether there will be extra APIs to accomodate for bitfields.

[dsnopek]

Discussed at the GDExtension, and we decided to:

• Document that for bitfields, the constant value should be treated as a uint64_t, and recommend that GDExtension bindings attempt to warn developers who use a signed value as the source of a bitfield constant (which the GDExtension interface can't check, because it takes a int64_t anyway). @Bromeon is going to make a PR to add this note to the documentation for classdb_register_extension_class_integer_constant in gdextension_interface.h
• Change the BIND_BITFIELD_FLAG() macro in godot-cpp to expect a uint64_t value so that developers get a compiler warning if they use an int64_t. @dsnopek can work on this change