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

Implement hypot.c #57

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Implement hypot.c #57

merged 1 commit into from
Feb 16, 2024

Conversation

AJenbo
Copy link

@AJenbo AJenbo commented Oct 29, 2023

This function is required for the DevilutionX port of Diablo, see diasurgical/devilutionX#6746

@JayFoxRox
Copy link
Member

Yes, looks good. Initially I was a bit skeptical about reusing other math.h functions in the implementation, but we already do it for others (like exp in sinh etc.)


However, the commit name is missing the "xbox: " prefix.
I'll merge this as soon as the commit is renamed (if nobody else does it)

@XboxDev I'm not sure if GitHub allows renaming in the web-ui with "Squash and merge" for single commits, but we could try enabling that option to avoid holding back PRs like this (due to minor style issues).

@GXTX
Copy link

GXTX commented Oct 30, 2023

Yes, looks good.

Deviates from the standard.

Page 273: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf

The hypot functions compute the square root of the sum of the squares of x and y , without undue
overflow or underflow. A range error occurs for some finite arguments.

Which is why it's a separate function from just sqrt.

@AJenbo
Copy link
Author

AJenbo commented Oct 30, 2023

GitHub does allows renaming in the web-ui with "Squash and merge" for single commits, which would be a lot easier then for me to do it 😅

@JayFoxRox
Copy link
Member

Which is why it's a separate function from just sqrt.

Yes. We should create an issue after merge (about the precision).
I can tell it should be higher precision (like Fused Multiply-Add) and can avoid precision errors.

Here, on x86/x87, the intermediate might be calculated in higher precision (80 bits), so it probably avoids a bunch of issues (except for hypotl which actually demands that precision).

Eitherway, the proposed implementation is still good enough (to me): it's better than just assert'ing.

@AJenbo
Copy link
Author

AJenbo commented Oct 30, 2023

However, the commit name is missing the "xbox: " prefix. I'll merge this as soon as the commit is renamed (if nobody else does it)

Fixed

@AJenbo
Copy link
Author

AJenbo commented Feb 16, 2024

Is there anything missing before this can progress?

We are also lacking an implementation for strtod() for the upcoming release version since it is needed by Lua which are are adding as a way to mod the game.

@thrimbor
Copy link
Member

Just a lack of time on my side, sorry.
As for stdtod(), you'll have to provide your own implementation (or take one from a third party that you can just drop in) temporarily.

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

Successfully merging this pull request may close these issues.

None yet

4 participants