Float conversion with dynamic scaling

[Okarss]

This is a complete rewrite of the floating point code, which introduces the improvements discussed in #221 and #223. The main new features:

• Introduces a configurable 64/32/16/8-bit unsigned integer width for conversion arithmetic, therefore removing the necessity for 64-bit integer arithmetic and eliminating approximately 1 KB of compiled code for 32-bit CPUs and up to 2 KB for even smaller CPUs.
• Introduces a configurable conversion character buffer size for printing numbers with more than 22 digits.
• Removes the range limitations.
• Supports subnormal numbers.
• Improves accuracy.
• Processes the double type directly instead of converting it to float.
• Does not calculate more digits than requested.

[charlesnicholson]

This looks great, and the size tests don't lie :)

I left feedback about updating the comment; please also update the readme.md to take credit for your work, and also to explain it! :)

[Okarss]

I will add commits for the documentation and more detailed comments later, when the code will be in it's final state. Also I will add new tests in unit_ftoa_rev.cc for the rewritten npf_ftoa_rev() function. Actually there are a bunch of algorithm specific edge cases, which can be tested, so there are quite a few tests to come up with.

https://en.cppreference.com/w/c/language/_Static_assert
The _Static_assert introduced by C11 has a problem that it is already deprecated and replaced by static_assert in C23. Similar to _Alignas/alignas, _Bool/bool and others... They finally came to the senses! Though in real life the static_assert has been working for quite some time already, at least with GCC. I can add it, but honestly, at least for now, I would not introduce this dependency just for this single number.

Another open question is what integer widths the unit tests should test. For now I set it to 32-bit, but probably it's worth considering some tests for 64-bit and maybe even 16/8-bit tests at least for a single platform. Actually I added the same compiler warnings to my local setup and for 8-bit integers the Wconversion popped up a bunch of warnings because those integers are smaller than the C's forced int type. I already patched it locally, but unfortunately this turns even a simple man_f += 3 into a messy man_f = (npf_ftoa_man_t)(man_f + 3).

Also some C++ tests on Clang failed with this:

nanoprintf.h:542:21: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
  char const *ret = NULL;
                    ^~~~
                    nullptr

https://en.cppreference.com/w/cpp/types/NULL
If that compiler doesn't like zero as a pointer constant, then why doesn't the accompanying header file define NULL as a nullptr? Seems inconsistent to me... Should we #undef NULL and redefine it for C++!?

[charlesnicholson]

Re: static_assert in C23- wow, I hadn't seen that, it's lovely (and overdue) that they're finally unifying it across C and C++!

Re: clang yelling about NULL, I'd rather just throw that on the heap of warnings that we shut clang up with. I guess we could alternately define NPF_NULL internally or something, but that feels a little like busywork just to placate clang. (I do agree with the shift from 0 to nullptr but this is a C library that happens to be C++-compatible; there are no other C++ idioms present, etc.)

Re: -Wconversion, I think that's sadly the cost of doing business, a lot of the code gets uglier but IMO it's a good warning...

Re: tests- I don't mind the build getting bigger / more complex if it means more test coverage! Maybe a separate nested loop in the cmake test file that runs the same tests with 8/16/32, and at test runtime uses define flags to figure out when to stop (i.e. so we don't run 32-bit width tests in the 8-bit config?)

[charlesnicholson]

Oh neat, look at that! https://en.cppreference.com/w/c/language/nullptr

Well, when it's time to move npf to C23, I'll delete the clang-suppression warning :)

[Okarss]

Here is a patch for paland.cc: float32.patch
I checked all of the float values locally, therefore, with the patch applied, all of those tests should be passing.

Another potential minor issue is that both the current release and this rework prints the sign for NAN and OOR outputs. Though for OOR it somewhat even makes sense in a similar way as for INF.

[charlesnicholson]

I applied the patch to a remote branch named okarss-float; let me know if you have any problems moving the submodule pointer to that branch!

[Okarss]

As all of the current checks are passing, now I will create some useful unit tests for the new npf_itoa_rev(). I guess, at least for now I will create those for 32-bit integer math, and then we can probably split off and expand the relevant ones for other integer widths. What is the functional difference between the TEST_CASE() and SUBCASE() macros?

[charlesnicholson]

Exciting! The difference between TEST_CASE and SUBCASE is that SUBCASE is an inner block, and each SUBCASE is executed in isolation when testing- the entire TEST_CASE is re-run, but only the "current" SUBCASE is executed; the rest are skipped. So, all of these tests will pass:

TEST_CASE("demonstrate subcases") {
  int i{ 0 };
  SUBCASE("a") {
    ++i;
    REQUIRE(i == 1);
  }
  SUBCASE("b") {
    ++i;
    REQUIRE(i == 1);
  }
  SUBCASE("c") {
    ++i;
    REQUIRE(i == 1);
  }
}

[Okarss]

By the way, all of the currents tests run on platforms, where the size of double type is 64-bit. The code itself works even with 32-bit double and theoretically should work even with 16-bit double, although I don't even know such a platform. I tested manually the 32-bit double functionality with Arduino IDE on AVR platform and it seems to work fine. If at some point in the future we intend testing this feature thoroughly, we will need some compiler/platform, which has 32-bit double type - AVR, MSP430, RL78 (configurable) or similar. Now back to the main topic...

I went further and already created tests for all integer widths. I used the CHECK() macro instead of the REQUIRE(), as that doesn't cancel the whole test case in case of some failure and one can see the results of all tests. Was pretty surprised about std::reverse() not existing with std=gnu++17. Not even to mention the fact that after half of a century the C standard library still has no similar function at all... Another thing I noticed is that 10 of the 13 unit tests disable GCC/Clang warnings individually and those are mostly the same warnings. Maybe we can just merge those pragmas in unit_nanoprintf.h like it is already done for MSVC?

Any comments on the newly added unit tests?

[charlesnicholson]

I think it all looks excellent, thanks again for the amazing work. Please credit yourself in the code (maybe with an algo description?) and in the readme- this is a huge effort and your name should be all over it :) As soon as you're ready to promote the PR out of draft, I'll happily approve!

[charlesnicholson]

BTW if you like, I keep meaning to move npf to C++20 for the test stuff. If you want to move it up in this PR, that's cool, otherwise I'll do it separately soon.

[charlesnicholson]

Hi @Okarss! Just checking in and saying hi. How is this PR coming? I'm very excited to merge it! No rush or anything; npf has no need to move quickly or with urgency at this point, but your PR is tantalizing and such an improvement that I'd love to get it in, so I'm curious :)

[Okarss]

Hi! I do completely understand your excitement and actually I myself am pretty intrigued. I pushed an updated documentation. Actually it would be nice if you can check and correct the grammatical and style errors, as obviously English is not my first language. :)

I guess there is no need for C++20 right now and we can update to it later.

One more minor issue I noticed. As this PR removes the range limits, the message "oor" (out of range) doesn't seem appropriate anymore. Now only the buffer size can cause a non-standard error situation. Maybe that message should be changed to for example "buf" or even a generic "err"?

[charlesnicholson]

Your English is apparently better than mine; thank you for the grammar fixes! I always forget to hyphenate adjective clauses. :)

I agree that oor no longer makes sense, I think err is probably ok but if you feel strongly about buf, my preference isn't strong. To me, err is clearer but less specific, and users might be confused by buf despite it being more precise. This is all minor, of course.

The PR otherwise looks great. Feel free to open it whenever you're ready!

[Okarss]

The buf was the first idea, but, thinking more about it, I also think the err is better. The users will understand it almost subconsciously and, being less specific, makes it also more future-proof if some other errors appear at some point. I adjusted this and basically we are good to go. Just one more thing - I guess you have to merge the paland conformance test branch to the main and then I can also retarget this PR to the main with the next commit.

[charlesnicholson]

I've just merged your branch over in paland-conformance to main, so you should be all set for retargeting!

[Okarss]

In addition I improved the algorithm explanation - it is more appropriate and easier to understand now. If necessary, I can also merge the changes from main into this PR, but I guess it's not necessary. Otherwise this PR is finally ready to go. :)

[charlesnicholson]

I have the branch protections set to require that the PR branch be up-to-date with the main branch, so that the tests that run cover what "main will become" if the PR merges. I clicked the "update branch" button on your behalf :)

[Okarss]

So, from my side everything is done. Or is there something else left to do?

Funny moment... I guess for this topic, the nanoprintf is the answer, especially the upcoming version with this PR.

By the way, now I have a few more ideas how to decrease the code size for other parts of the code. :D But, of course, those will not be as drastic as this one...

[charlesnicholson]

🥳

Thanks again for such a high-quality and novel code. It's awesome.

I would absolutely love to hear any ideas you have about the code footprint; I think at this point that you're probably as knowledgable of the npf internals as anyone 😄 Let's talk about it over in discussions, or if you want to open a new issue to talk it over, or whatever!