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

Corrections for printf test with floating point limits arguments #1940

Merged
merged 4 commits into from
May 21, 2024

Conversation

shajder
Copy link
Contributor

@shajder shajder commented Apr 8, 2024

According to work plan from issue #1058

@shajder shajder marked this pull request as ready for review April 9, 2024 06:43
@svenvh
Copy link
Member

svenvh commented Apr 10, 2024

Thanks! Would you be able to provide some more details on what corrections were made here? I see a few changes to the testing scope, but not sure if I've caught all of them.

@shajder
Copy link
Contributor Author

shajder commented Apr 10, 2024

Thanks! Would you be able to provide some more details on what corrections were made here? I see a few changes to the testing scope, but not sure if I've caught all of them.

Yes, of course:

-applied suggestions from @franz , I've done some brief research and I think Michał has a point at (1) it is not certain that sqrt(-1.0) return -NaN, thus replaced with float nan(uint nancode). At point (2) Michał has partialy the point because current verification of test result ignores sign of NaN anyway, thus there is no point to distinguish between positive and negative NaN.

-strengthening the test by removing allowance of Inf/1.#IND00/NaN - according to the spec only INF, INFINITY, or NAN for uppercase conversion format or inf, infinity, or nan for lowercase conversion format are permissible.

-extending the frameworks functionality to test multiple conversion formats for the same input data with the same expected output, eg. entry:
{ { "%f", "%e", "%g", "%a" }, "1.0f/0.0f" }
schedule 4 tests expecting the same result inf or infinity (the same applies for single and half float).

-rest of corrections comes from clang-format sadly, I think there is no way around (except putting clang-format off comments which I am trying not to abuse)

svenvh
svenvh previously approved these changes Apr 11, 2024
Copy link
Member

@svenvh svenvh left a comment

Choose a reason for hiding this comment

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

Thanks for the summary, that's very helpful. The changes look good to me.

test_conformance/printf/util_printf.cpp Outdated Show resolved Hide resolved
svenvh
svenvh previously approved these changes Apr 22, 2024
@lakshmih
Copy link
Contributor

We're reviewing this. Thanks.

@shajder
Copy link
Contributor Author

shajder commented May 17, 2024

@svenvh FYI: the same problem with skipped vector cases applied to this PR as well, I've corrected it but you may want to review again, thanks.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

There are a lot of extraneous blank lines in util_printf.cpp. How would folks feel about removing them? This might be a good time to do it...

@svenvh
Copy link
Member

svenvh commented May 20, 2024

There are a lot of extraneous blank lines in util_printf.cpp. How would folks feel about removing them? This might be a good time to do it...

+1 for removing them, they're an eyesore. I would suggest doing that in a separate PR though (maybe after the other printf PR is merged too, to avoid merge conflicts).

@neiltrevett neiltrevett merged commit 88a707d into KhronosGroup:main May 21, 2024
7 checks passed
neiltrevett pushed a commit that referenced this pull request Jun 18, 2024
According to work plan from issue #1058

Corrections to general test:
-removed duplication of separate tests for each element of
`PrintfTestType` vector, instead `doTest` procedure would iterate over
vector related to specific `PrintfTestType` automaticaly
-fixed procedure to assemble kernel source so it can accept only one
parameter of the function ( eg. `printf("%%");` )
-incorporated important modifications from #1940 to avoid expected
conflicts
-warnings fixes, minor corrections, clang format

Extension for string testing:
-special symbols
-nested symbols
-all ascii characters 
-added new type of test `TYPE_FORMAT_STRING` to verify format string
only (according to request from the issue)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants