Skip to content

ignore the zero flag for nan/inf/err#303

Merged
charlesnicholson merged 5 commits into
mainfrom
ignore-zero-flag-nan-inf
May 5, 2025
Merged

ignore the zero flag for nan/inf/err#303
charlesnicholson merged 5 commits into
mainfrom
ignore-zero-flag-nan-inf

Conversation

@charlesnicholson

@charlesnicholson charlesnicholson commented May 4, 2025

Copy link
Copy Markdown
Owner

This PR is a reimplementation of #289 by @stefano-zanotti-88 at my computer so that I can iterate on tests.

@charlesnicholson

Copy link
Copy Markdown
Owner Author

Looks like this is failing for the -nan tests:
https://github.com/charlesnicholson/nanoprintf/actions/runs/14825050480/job/41617083320?pr=303#step:4:386

@stefano-zanotti-88 are you seeing this on your version?

@stefano-zanotti-88

Copy link
Copy Markdown
Contributor

I'm not familiar with the error reporting used by these tests. Can you determine which of these lines is the one failing?

require_conform("nan" , "%f", npf_nan(0, 1, 0));
require_conform("-nan", "%f", npf_nan(1, 1, 0));
require_conform("nan" , "%f", npf_nan(0, 1, 1));
require_conform("-nan", "%f", npf_nan(1, 1, 1));
require_conform("nan" , "%f", npf_nan(0, 0, 0));
require_conform("-nan", "%f", npf_nan(1, 0, 0));
require_conform("nan" , "%f", npf_nan(0, 0, 1));
require_conform("-nan", "%f", npf_nan(1, 0, 1));

My first guess is that the system printf is not respecting the sign, while we are.
See the standard (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf), on page 333:

A double argument representing an infinity is converted in one of the styles [-]inf or
[-]infinity — which style is implementation-defined. A double argument representing a
NaN is converted in one of the styles [-]nan or [-]nan(n-char-sequence) — which style, and
the meaning of any n-char-sequence, is implementation-defined. The F conversion specifier
produces INF, INFINITY, or NAN instead of inf, infinity, or nan, respectively.
[…]
When applied to infinite and NaN values, the -, +, and space flag characters have their usual meaning; the # and 0 flag
characters have no effect.

It's never explicit about what sign a NaN should have. The obvious thing is to respect the sign bit from the binary representation. However, the default NAN macro, as well as operations like 0.0/0, do not have a guaranteed NaN representation (sign nor mantissa), so maybe the standard is actually loose on this point in printf too? Or maybe, the system printf we are checking against performs some preliminary operations on the NaN we are passing in, which results in its sign/mantissa being changed in a compiler-dependent way? For instance, if we pass in a value v which is NaN, I don't think that even v+0 is guaranteed to result in exactly v, it could be any other NaN.

In the end, I don't think we should be checking against the system printf at all when it comes to nans, but only against one specific string among the ones that the standard allows. For instance, the system printf could also output "nan(0x8000000000000)", or "-nan(0x8000000000000)". Some non-conforming implementations also output "1.#QNAN" or "1.#SNAN"

Watch out for INFINITY too. The standard allows both "inf" and "infinity". However, in that case, the sign is certainly respected, and an infinity cannot be substituted by the other one by trivial operations.

@charlesnicholson

Copy link
Copy Markdown
Owner Author

Ugh, yeah, that seems to be what's happening. Since all of this stuff is so ill-defined, I think it's wrong to use the conformance stuff- the point of that code is to make sure that nanoprintf behaves identically to the system printf, to learn about divergences. But, I've seen 3 different NAN printing behaviors on a few different systems i've tested, so I'm going to change the tests to just ensure that nanoprintf prints what we feel it should.

@charlesnicholson

Copy link
Copy Markdown
Owner Author

I pulled all the new NaN / INF stuff out into a new file and fixed up the tests for it. They're all passing, it's much better.

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.

2 participants