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

Remove dependency on libm #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

caramelli
Copy link
Contributor

I'm just wondering if we could remove the dependency on libm.

Just a few changes to do this, but I could be wrong.

@tomolt
Copy link
Owner

tomolt commented Mar 24, 2024

Thanks a bunch. This looks got to merge, but it'll have to wait a bit longer because I want to make sure this won't break something, but I don't have the time to do that just now.

schrift.c Outdated
@@ -65,6 +64,7 @@
#define GOT_A_SCALE_MATRIX 0x080

/* macros */
#define ABS(x) ((x) > 0 ? (x) : -(x))
Copy link
Contributor

@marler8997 marler8997 Mar 25, 2024

Choose a reason for hiding this comment

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

To match the semantics of the glibc implemntation this should be ((x) >= 0 ? (x) : -(x)).

See: https://github.com/lattera/glibc/blob/master/stdlib/abs.c

As it's written, this would introduce -0 floating point values which might cause issues or changes in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting question. When in doubt, following the glibc implementation is probably the right choice.
So I updated the PR this way.

Choose a reason for hiding this comment

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

Aaand there IS difference between (x < 0 ? -x : x) and (x >= 0 ? x : -x). The former won't change the sign of a quiet NAN, the latter will :)

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.

4 participants