[libc-commits] [PATCH] D131023: [libc] add printf decimal float conversion

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:57
+  uint64_t carry = val.mul(small);
+  // due to the specifics of this program, we know that shift_amount will always
+  // be at least 128.
----------------
sivachandra wrote:
> Specifics of which program?
adjusted phrasing.


================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:115
+public:
+  PaddingWriter() { ; }
+  PaddingWriter(const FormatSection &to_conv, char init_sign_char) {
----------------
sivachandra wrote:
> Why is this required?
it throws an error if there is no `PaddingWriter` default constructor since `FloatWriter` has it as a member.


================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:364
+    // Also we need to write the exponent, but that's pretty simple.
+    return -1;
+  }
----------------
sivachandra wrote:
> Do you want it to be failure return always? AFAICT, it is being used below.
`write_last_block_exp` will be used for the `%e` conversion, but is currently unused. `write_last_block_dec` is the one being used below, and is implemented above.


================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:464
+        size_t blocks_before_decimal = i;
+        float_writer.init((blocks_before_decimal * BLOCK_SIZE) +
+                              (has_decimal_point ? 1 : 0) + precision,
----------------
sivachandra wrote:
> Will `init` be called multiple times in the outer for loop? If not, it feels like we can improve the flow here.
It will not. I've added a comment to explain a bit, but this loop is trying to find the highest non-zero digit before the decimal point. If it finds that digit, then we know exactly how long the number as a whole will be, since we know how many digits are remaining before the decimal point and the number of digits after the decimal point is just the precision. Those are the values the `FloatWriter` is being initialized with.


================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:515
+      }
+      // Temporary: j is usually around 128, and by shifting a bit, we push it
+      // to 128 or above, which is a slightly faster code path in
----------------
sivachandra wrote:
> What is temporary?
oops, forgot to clean out some comments.


================
Comment at: libc/src/stdio/printf_core/float_dec_converter.h:568
+        RET_IF_RESULT_NEGATIVE(
+            float_writer.write_last_block_dec(digits, maximum, round));
+        break;
----------------
sivachandra wrote:
> Seems like `write_last_block` is not called from all paths? `write_last_block` does nothing now so it doesn't matter may be. But, the logical view feels incomplete now.
as discussed above, there are two `write_last_block` functions and this one is actually complete. As for if it needs to be called on all paths, not quite. `write_last_block_dec` handles flushing the buffer, converting the last block into a string, and rounding. In the cases where we know that all of the final digits are zero, we can skip those steps and just call `write_zeroes`, which also flushes the buffer, and no rounding can be necessary since the trailing digits are all zero.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131023/new/

https://reviews.llvm.org/D131023