[libc-commits] [PATCH] D131301: [libc] add int to string for extended bases

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/integer_to_string.h:82
 
+constexpr size_t floor_log_2(size_t num) {
+  size_t i = 0;
----------------
Look like a good candidate for `clz` builtin, unless it's only used for compile-time constants.  In that case, it should be ok to bump this to C++20 and use `consteval`.  Otherwise, you'll need to add `inline` for this.


================
Comment at: libc/src/__support/integer_to_string.h:109
+  static_assert(
+      2 <= Base <= 36,
+      "BaseIntegerToString can only have a base between 2 and 36 inclusive.");
----------------
Can we really do this?  On the surface it looks equivalent to `int(bool(2 <= Base)) <= 36` which is always `true` since `int(bool) is either 0 or 1.


================
Comment at: libc/src/__support/integer_to_string.h:150
+public:
+  constexpr explicit BaseIntegerToString() { ; }
+  constexpr explicit BaseIntegerToString(T val) { convert(val, Base, true); }
----------------
why is the `;` needed?


================
Comment at: libc/src/__support/integer_to_string.h:154
+  constexpr explicit BaseIntegerToString(T val, size_t conv_base) {
+    convert(val, conv_base, true);
+  }
----------------
`convert(val, conv_base, /*lowercase=*/ true)` might be a bit easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131301