eyalroz / printf

Tiny, fast(ish), self-contained, fully loaded printf, sprinf etc. implementation; particularly useful in embedded systems.
MIT License
419 stars 52 forks source link

Two problems about the get_components function #170

Closed HenryLin2000 closed 3 months ago

HenryLin2000 commented 11 months ago

I found two problems with this code in lines 599-602 of the get_components function in printf.c. And I hope to discuss with you. https://github.com/eyalroz/printf/blob/f8ed5a9bd9fa8384430973465e94aa14c925872d/src/printf/printf.c#L599-L602 (1)Is there a lack of carry processing when the fractional rounding up? For example, 0.995 takes 2 decimal places, and the decimal place results in 100, which exceeds the limit of 2 significant digits, but there is no carry to the integer, which will cause an exception. I noticed that in the previous if block, for the case of remainder>0.5, carry processing is done when the decimal digit exceeds the significant digit. Should there be a similar processing here?

  if (remainder > 0.5) {
    ++number_.fractional;
    // handle rollover, e.g. case 0.99 with precision 1 is 1.0
    if ((double) number_.fractional >= powers_of_10[precision]) {
      number_.fractional = 0;
      ++number_.integral;
    }
  }

(2)What is the function of the second condition (number_.fractional == 0U)? I know that (number_.fractional & 1U) is used to implement banker's rounding, but what is the role of fractional==0 here? Can't we round down according to the banker's rounding rules? Will this cause error accumulation? Similarly, I noticed that in the latter if block, for the case of precision==0, there is no similar judgment of integral==0.

  if (precision == 0U) {
    remainder = abs_number - (double) number_.integral;
    if ((!(remainder < 0.5) || (remainder > 0.5)) && (number_.integral & 1)) {
      // exactly 0.5 and ODD, then round up
      // 1.5 -> 2, but 2.5 -> 2
      ++number_.integral;
    }
  }

Also, I found a similar issue in mpaland's repository mpaland/printf#51

HenryLin2000 commented 11 months ago

I tried calling printf on my computer (x86+windows+mingw) to output floating point numbers. If it is retained to an integer, the integer part is rounded towards an even number. If it is retained to n decimal places and the first n decimal places are all 0, the decimal part is rounded up. This behaves the same as printf.c in our repository. So problem 2 I mentioned may not be a problem.

But based on the above rules, for the lines 599-602 I mentioned, simply adding a carry will result in different behaviors when retaining to an integer, so I think the complete modification can be as follows:

  else if (remainder == 0.5) {
    if ((number_.fractional == 0U) || (number_.fractional & 1U)) {
      // if halfway, round up if odd OR if last digit is 0
      ++number_.fractional;
      // handle rollover after round up, e.g. case 0.995 with precision 2 is 1.00
      if ((double) number_.fractional >= powers_of_10[precision]) {
        number_.fractional = 0;
        ++number_.integral;
      }
    }
  }

  if (precision == 0U) {
    remainder = abs_number - (double) number_.integral;
    if ((!(remainder < 0.5) || (remainder > 0.5)) && (number_.integral & 1)) {
      // exactly 0.5 and ODD, then round up
      // 1.5 -> 2, but 2.5 -> 2
      ++number_.integral;
    }
    if (remainder == -0.5 && (number_.integral & 1)){
      --number_.integral;
    }
  }

Also, I found the original code of the 3rd if block strange, it seemed to never be executed based on the current conditions. A related issue: #153

eyalroz commented 11 months ago

It looks like you're looking at the master branch rather than the development branch. Please re-examine...

HenryLin2000 commented 11 months ago

Sorry, I didn't notice the develop branch before. But I found there is still a problem in develop branch. https://github.com/eyalroz/printf/blob/df06fd4d1b4deb1db649dc50d82a56488bca8b7b/src/printf/printf.c#L626-L630 For the second if block, it still doesn't handle integer carries when the decimal exceeds the digit limit. For example, when you call get_components(0.995, 2), you will get a number_ with fractional=100, which will make a mistake.

eyalroz commented 3 months ago

So, if I make the correction and apply the roll-over for this case:

-  if (remainder > one_half) {
+  if ((remainder > one_half) ||
+      // Banker's rounding, i.e. round half to even: 1.5 -> 2, but 2.5 -> 2
+      ((remainder == one_half) && (number_.fractional & 1U))) {
     ++number_.fractional;
-    // handle rollover, e.g. case 0.99 with precision 1 is 1.0
-    if ((floating_point_t) number_.fractional >= powers_of_10[precision]) {
-      number_.fractional = 0;
-      ++number_.integral;
-    }
   }
-  else if ((remainder == one_half) && (number_.fractional & 1U)) {
-    // Banker's rounding, i.e. round half to even:
-    // 1.5 -> 2, but 2.5 -> 2
-    ++number_.fractional;
+  if ((floating_point_t) number_.fractional >= powers_of_10[precision]) {
+    number_.fractional = 0;
+    ++number_.integral;
   }

I get output that conflicts with glibc's. It believes printf("%.2f",0.995) should yield 0.99, not 1.00. This is a bit of a conundrum :-(

... oh, wait!

actually, 0.995 is not even floating-point-representable. There is no 0.995, it's actually 0.994999999999999996 .