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

[Joseph Huber via Phabricator via libc-commits]

jhuber6 added a comment.

Just a few nits, I like it overall but will defer accepting to the standard `libc` contributors.



================
Comment at: libc/src/__support/FPUtil/fpbits_str.h:52-53
 
   s += " = (";
-  s += cpp::string("S: ") + (x.get_sign() ? "1" : "0");
+  s += "S: ";
+  s += sign_char(x.get_sign());
----------------



================
Comment at: libc/src/__support/StringUtil/signal_to_string.cpp:32
   // the buffer should be able to hold "Real-time signal" + ' ' + num_str
-  return (base_str_len + 1 + max_num_len) * sizeof(char);
+  return (base_str_len + 1 + int_buffer_size()) * sizeof(char);
 }
----------------
Do we need a helper for what is already a one-liner? I think it should fit without needing to be split to the next line.


================
Comment at: libc/src/__support/integer_to_string.h:193-195
+    constexpr auto max = [](size_t a, size_t b) -> size_t {
+      return a > b ? a : b;
+    };
----------------
Unrelated, but this really should probably go in some common `libc` header since there's another use in `rpc_client.h`.


================
Comment at: libc/src/__support/integer_to_string.h:94
+
+using BinString = details::Config<2>;
+using OctString = details::Config<8>;
----------------
gchatelet wrote:
> jhuber6 wrote:
> > I'm not a fan of the name, since this isn't a string it's a config for the radix. Could we do something like `radix::Decimal`, etc?
> The intent was that it could be read //naturally// : `IntegerTo<DecString::WithPrefix>` -> `Integer to dec string with prefix`.
> Now I'm fine with another approach.
> 
> I'd like to gather a few other opinions / suggestions before changing though.
I just feel like it's clearer if you have a function called `IntegerToString` which takes a radix config by template, e.g. `IntegerToString<radix::Decimal>`. It's more words but I just find it a little weird to 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156981