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

Michael Jones via Phabricator via libc-commits libc-commits at lists.llvm.org
Mon Aug 8 10:56:08 PDT 2022

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.

  rG LLVM Github Monorepo



More information about the libc-commits mailing list