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

Mixing int32 and int64 types in comparisons leads to errors (certainl… #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

FreeBear-nc
Copy link

…y on x86_64 builds)

Built a small test routine calling fixedpt_sqrt(x) where 0>x<1 with -DFIXEDPT_BITS=64 -DFIXEDPT_WBITS=14
Returned a pathological and incorrect result:
fixedpt_sqrt(0.070700)) = 7314.706333859 (expected 0.265895)

Comparison & conversions of int32 & int64 values to blame .

@cxw42
Copy link
Owner

cxw42 commented Jul 5, 2020

Thank you for this PR! Would you please also add a test that reproduces the issue on your system? Since you are using a specific bit configuration, I think a new file with just your test(s) would probably be best. Thanks!

@FreeBear-nc
Copy link
Author

Simple test added.
Shouldn't make much difference what value is used for FIXED_WBITS.

Should also point out that I'm compiling & testing on an x86_64 system. Same problem exists for arm64 builds - Don't have a native 32bit system to hand for testing.

@cxw42
Copy link
Owner

cxw42 commented Jul 16, 2020

Thank you! Looks reasonable to me - I'll test on my systems and then merge. I have the same platforms available as you do, but it occurs to me I could probably cross-compile for i686 and try that as well.

…values

of 8, 64, ...
The issue is down in part, to the return value of fixedpt_div being truncated
in the conversion from fixedptd to fixedpt
@cxw42
Copy link
Owner

cxw42 commented Oct 7, 2020

Sorry for the delay --- I haven't forgotten; I've just been very busy at work!

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