Skip to content

Switch to 64-bit ints.#1357

Merged
dsnopek merged 1 commit into
godotengine:masterfrom
bruvzg:cpp_64
Jan 19, 2024
Merged

Switch to 64-bit ints.#1357
dsnopek merged 1 commit into
godotengine:masterfrom
bruvzg:cpp_64

Conversation

@bruvzg

@bruvzg bruvzg commented Jan 10, 2024

Copy link
Copy Markdown
Member

Syncing features with godotengine/godot#86730, but since no GDExtension public API is changed, it should be compatible both ways.

@bruvzg bruvzg added enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation labels Jan 10, 2024
@bruvzg bruvzg added this to the 4.3 milestone Jan 10, 2024
@AThousandShips AThousandShips added the waiting for Godot PRs that can't be merged until an engine PR is merged first label Jan 10, 2024
@AThousandShips

Copy link
Copy Markdown
Member

Should fix:

@AThousandShips AThousandShips removed the waiting for Godot PRs that can't be merged until an engine PR is merged first label Jan 10, 2024
@bruvzg
bruvzg marked this pull request as ready for review January 10, 2024 21:41
@bruvzg
bruvzg requested a review from a team as a code owner January 10, 2024 21:41

@dsnopek dsnopek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

This looks great to me 🚀

I diff'ed the cowdata.hpp and vector.hpp from this PR with the cowdata.h and vector.h from PR godotengine/godot#86730, and they only had minor expected differences. Reading through all the rest, it all looks good.

I haven't tested it manually, but the fact that it's passing the automated tests is a very good sign.

@dsnopek

dsnopek commented Jan 19, 2024

Copy link
Copy Markdown
Collaborator

I know we can merge this before Godot, however, I'm holding off on merging for a bit, just in case there's any further changes to cowdata.h and vector.h on the Godot PR that we'll want to sync over here. When that one seems like it's finished and about to be merged, we can merge here.

@dsnopek

dsnopek commented Jan 19, 2024

Copy link
Copy Markdown
Collaborator

PR godotengine/godot#86730 was just merged. I diffed the vector.h and cowdata.h with godot-cpp's vector.hpp and cowdata.hpp from this PR again, and didn't see any further changes that need to be sync'd. So, I think this one is good to merge now!

@dsnopek
dsnopek merged commit 0145e90 into godotengine:master Jan 19, 2024
@dsnopek

dsnopek commented Jan 19, 2024

Copy link
Copy Markdown
Collaborator

Thanks!

@dsnopek

dsnopek commented Jan 22, 2024

Copy link
Copy Markdown
Collaborator

Cherry-picking to 4.2 in #1372

@dsnopek

dsnopek commented Jan 22, 2024

Copy link
Copy Markdown
Collaborator

Cherry-picking to 4.1 in #1373

@AThousandShips

Copy link
Copy Markdown
Member

Assuming this doesn't actually depend on the upstream changes? As they aren't cherry picked, we might want to be careful here as it does have some side effects that might be harmful:

I'd hold off cherry picking this until we have worked things out

@bruvzg

bruvzg commented Jan 22, 2024

Copy link
Copy Markdown
Member Author

It doesn't depend on upstream change, but won't fix any of the issues without them, so is not useful on its own.

template <class T>
class CharStringT;

SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This specifically breaks on some platforms, still being investigated

@YuriSizov

Copy link
Copy Markdown
Contributor

Should fix:

Did it fix this issue btw?

@dsnopek

dsnopek commented Jan 24, 2024

Copy link
Copy Markdown
Collaborator

Should fix:

Did it fix this issue btw?

Yes it did! I've just closed that issue.

@dsnopek

dsnopek commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

Cherry-picked for 4.2 in PR #1410

@dsnopek

dsnopek commented Mar 11, 2024

Copy link
Copy Markdown
Collaborator

Cherry-picked for 4.1 in PR #1411

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

Labels

enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants