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

Changed AlignedAllocate to default to posix_memalign #738

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Nov 3, 2023

I'm in the process of implementing iOS support for Godot Jolt, and it turns out that aligned_alloc is only available on iOS >=13, as seen here. According to that header it also appears that aligned_alloc is not available on macOS versions prior to 10.15 (Catalina).

What is available however (also found in the link above) is posix_memalign.

I figured instead of adding even more platform ifdef's to AlignedAllocate I would just simplify things a bit and always default to posix_memalign on anything but Windows, as this seems to be available on Android as well. Although, I'm not sure how this change fares on that one blue platform.

I ran PerformanceTest on macOS and saw no difference compared to aligned_allocate.

Also note that posix_memalign technically returns an error code, but seeing as how none of the other allocation functions had any error-handling I opted to just ignore it.

Jolt/Core/Memory.cpp Outdated Show resolved Hide resolved
@jrouwe
Copy link
Owner

jrouwe commented Nov 3, 2023

Note that I just fixed the Shared library compiler errors as part of #739 (github must have bumped a compiler version).

@mihe
Copy link
Contributor Author

mihe commented Nov 3, 2023

Alright, I guess either of us can merge in master here once #739 has been merged, if you'd prefer to see a successful run first.

@jrouwe jrouwe merged commit a58e548 into jrouwe:master Nov 3, 2023
62 checks passed
@jrouwe
Copy link
Owner

jrouwe commented Nov 3, 2023

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants