[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:166
+    convert(val, conv_base, lowercase);
+  }
+
----------------
sivachandra wrote:
> Based on the use case you have in D131302, a better design seems to be to use a template function and not a template class?
> 
> ```
> class IntegerToString {
> public:
>   template <typename T, uint8_t BASE>
>   constexpr size_t bufsize() {
>     return ...; // The BUFSIZE formula
>   }
> 
>   template <typename T, uint8_t BASE,  cpp::enable_if_t<2 <= BASE && BASE <= 10, int> = 0>
>   static constexpr cpp:optional<cpp::StringView> convert(T val, cpp::MutableArrayRef<char> &buffer) {
>     // If This function can actually be a constexpr, then the below "if" will be optimized out.
>     if (buffer.size() < bufsize<T, BASE>())
>       return cpp::optional<cpp::StringView>();
> 
>     // Perform conversion and write it to buffer
> 
>     return cpp::StringView(...); // A StringView into the |buffer|
>   }
> 
>   template <typename T, uint8_t BASE, cpp::enable_if_t<10 < BASE && BASE <= 36, int> = 0>
>   static constexpr cpp:optional<cpp::StringView> convert(T val, cpp::MutableArrayRef<char> &buffer, bool lowercase) {
>     ...
>   }
> 
> };
> 
> 
> ```
since the goal of that patch is to shrink the code size, I designed it to avoid templating the convert function because that would result in three copies of near-identical code with different bases. I do like the idea of using `optional` to wrap the `StringView` though, since that solves the problem of returns. I will consider the problem more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131301