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

Fix bool type appearing in GDExtension header #96406

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

Bromeon
Copy link
Contributor

@Bromeon Bromeon commented Aug 31, 2024

Repetition of the exercise in #71696 and #81247.

Since 3 is a good number of times to start automating, I'll look into preventing this.

@Bromeon Bromeon force-pushed the bugfix/bool-in-header branch from 9b85b20 to 82b2a58 Compare August 31, 2024 20:40
@Bromeon Bromeon requested a review from a team as a code owner August 31, 2024 20:40
@dsnopek
Copy link
Contributor

dsnopek commented Aug 31, 2024

Eep, sorry! This could be helped by the idea for generating the header from JSON/XML, because we could have a list of allowed types. I'm going to try to explore that in the 4.4 dev cycle if I find the time, although, it's not super high priority.

Anyway, this change looks good! Thanks!

@AThousandShips AThousandShips added this to the 4.4 milestone Sep 1, 2024
@Bromeon
Copy link
Contributor Author

Bromeon commented Sep 1, 2024

This could be helped by the idea for generating the header from JSON/XML, because we could have a list of allowed types. I'm going to try to explore that in the 4.4 dev cycle if I find the time, although, it's not super high priority.

Just running the header through a C compiler already catches most of these issues, which is what I did in #96408. Would be nice to have some validation like this soon, because incorrect headers keep breaking downstream projects 😉

@fire
Copy link
Member

fire commented Sep 2, 2024

Ok to merge?

@akien-mga
Copy link
Member

Ok to merge?

Yes, but also ok to have a weekend :)

@akien-mga akien-mga merged commit 04f1977 into godotengine:master Sep 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants