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

Add missing int→Variant conversions #1298

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Nov 4, 2023

Implements int8_t, uint8_t, int16_t, and uint16_t operations/constructors for Variant. This matches the implementation of the main repo (albeit using the stdint.h names), and allows binding methods that use these types as parameters/returns

@Repiteo Repiteo requested a review from a team as a code owner November 4, 2023 21:56
@Repiteo Repiteo force-pushed the int-to-variant-fix branch from 70e6512 to 553fa08 Compare November 4, 2023 22:05
@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 4, 2023

Also added signed long and unsigned long behind a NEED_LONG_INT define like the main repo. Frankly, I don't know how imperative this was, as the define is only used in iOS builds which I don't even think GDExtension supports, but the other int types were being ported over anyway so might as well

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 5, 2023

Thanks! This makes sense to me

Also added signed long and unsigned long behind a NEED_LONG_INT define like the main repo. Frankly, I don't know how imperative this was, as the define is only used in iOS builds which I don't even think GDExtension supports, but the other int types were being ported over anyway so might as well

GDExtension does support iOS builds, however, we don't ever define NEED_LONG_INT in the godot-cpp build system. Looking at Godot's build system it's defining it on iOS with arm64, so we should probably update the build system here to do the same.

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 6, 2023

This will need a rebase after PR #1299 in order for tests to pass

@Repiteo Repiteo force-pushed the int-to-variant-fix branch 3 times, most recently from f0e4ddb to 45298ca Compare November 6, 2023 14:48
@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 6, 2023

Rebased the repo & updated the iOS build system to use NEED_LONG_INT! I've never built for iOS so I wouldn't know how to test the change, but I included the define where it would reasonably be expecting an arm64 architecture. Because of the added "universal" option, the check was tweaked to ensure it's explicitly not x86_64

@dsnopek dsnopek requested a review from bruvzg November 8, 2023 15:29
@dsnopek
Copy link
Collaborator

dsnopek commented Nov 8, 2023

Thanks! This looks good to me, but I'm also not very experienced with iOS.

@bruvzg Do the iOS bits make sense to you?

src/variant/variant.cpp Outdated Show resolved Hide resolved
@Repiteo Repiteo force-pushed the int-to-variant-fix branch from 45298ca to bcac96c Compare November 8, 2023 15:46
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good to me :-)

@dsnopek dsnopek added bug This has been identified as a bug cherrypick:4.1 labels Nov 15, 2023
@dsnopek dsnopek modified the milestones: 4.2, 4.x Nov 15, 2023
@dsnopek dsnopek merged commit c4b7b08 into godotengine:master Nov 15, 2023
12 checks passed
@Repiteo Repiteo deleted the int-to-variant-fix branch November 15, 2023 21:59
@akien-mga akien-mga modified the milestones: 4.x, 4.2 Dec 1, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.1 in #1373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants