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

[Michael Jones via Phabricator via libc-commits]

michaelrj 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;
----------------
lntue wrote:
> 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.
it is used only for compile time constants. I don't know if we want to bump this to C++20, so I've marked it as constexpr inline for the moment. I think you're right that this could use `clz`, but since it's only at compile time I don't think that's necessary.


================
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.");
----------------
lntue wrote:
> 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.
you're right, that doesn't work. Fixed.


================
Comment at: libc/src/__support/integer_to_string.h:123
+    if (conv_base < Base) {
+      return;
+    }
----------------
sivachandra wrote:
> Silently return? Can we perhaps return a boolean value to indicate success/failure?
we could, but this function is only called from the constructor so I'm not sure what we'd do with that information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131301