Skip to content

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?

@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
@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

4 participants