[libcxx-commits] [PATCH] D97705: [RFC][libc++] Improve std::to_chars for base != 10.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Still some scattered `#if 0/1`s left to remove.



================
Comment at: libcxx/include/charconv:328
+struct _LIBCPP_HIDDEN __integral<2> {
+  enum : unsigned { _Base = 2 };
+
----------------
The red flag here was the `: unsigned` (I was like, "why does the signedness matter? better study closer") but actually, why do we need an enumerator for this at all? It's used only twice, and in contexts like
```
      _VSTD::memcpy(__p, &__base_2_lut[4 * __c], 4);
~~~
      unsigned __c = __value % _Base;
      __value /= _Base;
      *--__p = "01"[__c];
```
The number `2` is hard-coded so many places here, two more won't make a difference (and will save two lines and eliminate a red flag). Ditto in the `8` and `16` cases.

(If this was to appease clang-tidy: Don't appease it. :))


================
Comment at: libcxx/include/charconv:333
+    // If value == 0 still need one digit. If the value != this has no
+    // effect since the code scans for the most significat bit set. (Note
+    // that __libcpp_clz doesn't work for 0.)
----------------
I'd remove `noexcept` from line 331, as it's just noise. (Nobody relies on the noexceptness of this function.) Ditto throughout — I assume there will be other redundant `noexcept`s.


================
Comment at: libcxx/include/charconv:357
+      __value /= _Base;
+      *--__p = "01"[__c];
+    } while (__value != 0);
----------------
Is Clang smart enough to optimize this into `'0' + __c`? Would the latter be more readable anyway? (Note there is no issue with EBCDIC here; all charsets must have `'0'+1 == '1'` according to the standard.)
Ditto line 413.


================
Comment at: libcxx/include/charconv:362
+
+  static _LIBCPP_CONSTEXPR char __base_2_lut[64] = {
+    '0', '0', '0', '0',
----------------
Either `constexpr` if this is C++11-and-later, or `_LIBCPP_CONSTEXPR const`, surely?


================
Comment at: libcxx/include/charconv:489
+template <unsigned _Base, typename _Tp,
+          typename enable_if<(sizeof(_Tp) >= sizeof(unsigned)), int>::type = 0>
+_LIBCPP_AVAILABILITY_TO_CHARS _LIBCPP_INLINE_VISIBILITY int
----------------
`__enable_if_t<sizeof(_Tp) >= sizeof(unsigned)>* = nullptr` is the more traditional libc++ism. (`enable_if_t` if this is C++14-and-later.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97705