[libc-commits] [PATCH] D125929: [libc] add printf base 10 integer conversion

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf_core/int_converter.h:24
+// it's overestimating, but not by a lot, and it means it'll support any size.
+template <typename T> constexpr size_t digits_to_store() {
+  return (((sizeof(T) * 8) + 2) / 3);
----------------
Seems like this one line function is used only once?


================
Comment at: libc/src/stdio/printf_core/int_converter.h:38
+  static constexpr size_t BITS_IN_NUM = sizeof(uintmax_t) * BITS_IN_BYTE;
+  static constexpr size_t buff_len = digits_to_store<uintmax_t>();
+  uintmax_t num = to_conv.conv_val_raw;
----------------
`BUFF_LEN`


================
Comment at: libc/src/stdio/printf_core/int_converter.h:39
+  static constexpr size_t buff_len = digits_to_store<uintmax_t>();
+  uintmax_t num = to_conv.conv_val_raw;
+  char buffer[buff_len];
----------------
Can this assignment ever lead to a truncation? May no because this is a `convert_int` which does not deal with integers wider than 64-bit?


================
Comment at: libc/src/stdio/printf_core/int_converter.h:59
+  case LengthModifier::none:
+    num = num & type_mask<int>();
+    break;
----------------
Why are these masking operations required? Also, for max values, we have: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/CPP/Limits.h


================
Comment at: libc/src/stdio/printf_core/int_converter.h:90
+  size_t buff_cur = buff_len - 1;
+  while (num > 0 /* && buff_cur > 0 */) {
+    char new_digit = (num % 10) + '0';
----------------
```
for (; num > 0; --buff_cur, num /= 10)
  buffer[buff_curr] = (num % 10) + '0';
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125929