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

ConnectFlags::ord() and connect_ex().flags() should have a consistent type. #503

Closed
Lemiczek opened this issue Nov 27, 2023 · 3 comments · Fixed by #524
Closed

ConnectFlags::ord() and connect_ex().flags() should have a consistent type. #503

Lemiczek opened this issue Nov 27, 2023 · 3 comments · Fixed by #524
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals

Comments

@Lemiczek
Copy link
Contributor

I've been looking at connect_ex and noticed a curious thing:

let flag = ConnectFlags::CONNECT_ONE_SHOT;
node.connect_ex("some_signal".into(), callable)
     .flags(flag.ord() as u32) // <--- convert to u32?
     .done();

Why does ConnectFlags::ord return i32 when realistically it should only be u32?
If the Godot team ever decided to use signed integers on an enum (for some reason), we'd have to change the current .flags(value: u32) argument type anyway.

Feels like an unnecessary conversion that could be possibly dealt away with? 🙂

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: engine Godot classes (nodes, resources, ...) labels Nov 27, 2023
@Bromeon
Copy link
Member

Bromeon commented Nov 27, 2023

I think this is related to #185 and an original implementation #342, upon which we can improve.

Basically, users should not need to explicitly call ord. If we detect a parameter type to be flags, they should be able to pass a flag directly, or an OR-combination thereof.


If the Godot team ever decided to use signed integers on an enum (for some reason)

It's not "ever", this is already the case, so we have to keep enums signed. But flags are not enums.

@Bromeon
Copy link
Member

Bromeon commented Nov 28, 2023

Related: godotengine/godot-cpp#1320 just changed to consistently use u64 for all flag/bitfield types.

We should probably do the same.

@Lemiczek
Copy link
Contributor Author

Lemiczek commented Nov 28, 2023

Sounds good!

Having it be explicitly u64 for bitfields solves this issue, as we would only have the expectation of that type and no other. That way, consistency can be achieved 👍

Currently, it is my understanding that enum ordinals are i32 (or i64 on the public interface) regardless of whether they are a bitfield or not. So we'll have to make an edge-case for the bitfield type in codegen, alongside making methods that take a bitfield take a u64 instead of (in this specific case) a u32.

Should be a fairly easy, non-intrusive change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants