[libc-commits] [PATCH] D156981: [libc] Better IntegerToString API

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

Overall LGTM, I like the idea of the buffer being provided by the class and therefor guaranteed to be the right size. I left a few comments, but I think this is the right design moving forward.



================
Comment at: libc/src/__support/integer_to_string.h:90
+  // Invariants
+  static_assert(BASE == 2 || BASE == 8 || BASE == 10 || BASE == 16,
+                "Invalid radix");
----------------
why limit this to only these bases? Without the asserts this class would support any base from 2-36. Additionally, that would mirror how the string to integer functions work.


================
Comment at: libc/src/__support/integer_to_string.h:197
+    constexpr size_t digit_size = max(max_digits(), Fmt::MIN_DIGITS);
+    constexpr size_t sign_size = Fmt::BASE == 10 ? 1 : 0;
+    constexpr size_t prefix_size = Fmt::PREFIX ? 2 : 0;
----------------
why are signs only supported in base 10?


================
Comment at: libc/src/__support/integer_to_string.h:229
+        is_negative = value < 0;
+        write_number(is_negative ? -value : value, sink);
+      } else {
----------------
is there handling for `value` being the lowest negative value for a signed type (e.g. `LONG_MIN`)? In that case `-value` is equivalent to `value`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156981