Skip to content

Conversation

@jmid
Copy link
Collaborator

@jmid jmid commented Jul 28, 2025

In #565 I discovered that STM argument and result printers differed causing a negative nan to display as nan as an argument to Set, yet it would print as -nan in the result, sending me on a goose chase. #562 was another example of outputs differing slightly.

Following the principle of least surprise, this PR switches all Util.Pp printers for base types to use the QCheck.Print combinators. I believe this further makes sense, since our opam packages are named with a qcheck prefix.

I don't claim that, e.g., the QCheck.Print.float printer is perfect as witnessed by the expect test output updates (e.g., infinity is valid copy-pasteble OCaml, whereas inf is not) but I would rather have to only improve such a combinator in a single, central location.

@jmid
Copy link
Collaborator Author

jmid commented Jul 28, 2025

Ah dang, a negative nan is just printed as nan on macOS, MinGW, and MSVC it seeems, but as -nan on Linux, Cygwin, musl, ... 🤷

ddcebf1 removes the negative nan expect test case (temporarily) in order to move forward with the PR.

@jmid
Copy link
Collaborator Author

jmid commented Jul 28, 2025

I don't claim that, e.g., the QCheck.Print.float printer is perfect as witnessed by the expect test output updates (e.g., infinity is valid copy-pasteble OCaml, whereas inf is not) but I would rather have to only improve such a combinator in a single, central location.

I've opened an upstream issue on QCheck here: c-cube/qcheck#353

@jmid
Copy link
Collaborator Author

jmid commented Jul 28, 2025

CI summary for ddcebf1: All 50 workflows passed

@jmid jmid merged commit 06a83b9 into main Jul 28, 2025
50 checks passed
@jmid jmid deleted the util-pp-printers branch July 28, 2025 18:36
@jmid
Copy link
Collaborator Author

jmid commented Jul 29, 2025

CI summary for merge to main:

Out of 51 workflows 1 workflow failed with (what appears to be) a genuine issue

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