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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/integer_to_string.h:166
+    convert(val, conv_base, lowercase);
+  }
+
----------------
michaelrj wrote:
> 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.
You can keep the business logic in a single non-template function and make only the user facing functions templates. As it stands, this patch is going to trigger multiple copies of `convert` for the combinations of T x BASE?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131301