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

Fix tests failing for 32-bit targets #593

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

Conversation

grisumbras
Copy link
Member

@grisumbras grisumbras commented Jul 6, 2021

When an integer is converted to a floating point type which cannot hold it without loss of precision, the conversion is implementation-defined. This apparently means, that the conversion can result in slightly different results for the same number in different functions. Because of this several tests were failing for some 32 bit targets with optimizations enabled (#576). This change removes those tests and instead adds an equivalent test that uses an integer value that can be converted to float without loss of precision.

The question is, should we remove other similar tests that involve lossy conversions? Also, maybe instead of removing those tests, we should check that the results are within and allowed interval?

@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #593 (ccf41db) into develop (745be88) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #593   +/-   ##
========================================
  Coverage    99.05%   99.05%           
========================================
  Files           68       68           
  Lines         6114     6114           
========================================
  Hits          6056     6056           
  Misses          58       58           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 745be88...ccf41db. Read the comment docs.

@cppalliance-bot
Copy link

@vinniefalco
Copy link
Member

Did this get resolved? Or merged?

@grisumbras
Copy link
Member Author

No, I'm working on an alternative that doesn't simply drop EQUF tests, but replaces them with different tests.

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

3 participants