[libcxx-commits] [PATCH] D103433: [libc++][format] Adds integer formatter.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added a comment.

LGTM



================
Comment at: libcxx/include/__format/formatter_integral.h:77-78
+ *   from the C++-type group typedef. Most specializations need nothing else.
+ *   Others need some additional specializations in this class. For example the
+ *   128-bit integral formatters use the 64-bit version to do the formatting.
+ */
----------------
I suggest removing the "For example the 128-bit integral formatters use the 64-bit version to do the formatting." because it's a workaround that will go away per TODO in `format(__int128_t __value, auto& __ctx)` rendering this example wrong. In general I don't think there is a good reason for one specialization to reuse another.


================
Comment at: libcxx/include/__format/formatter_integral.h:313-319
+      if (__value == 0)
+        return __format_unsigned_integral(__array.begin(), __array.end(),
+                                          __value, __negative, 8, __ctx,
+                                          nullptr);
+      else
+        return __format_unsigned_integral(__array.begin(), __array.end(),
+                                          __value, __negative, 8, __ctx, "0");
----------------
Why not use a conditional expression?

```
        return __format_unsigned_integral(__array.begin(), __array.end(),
                                          __value, __negative, 8, __ctx, __value != 0 ? "0" : nullptr);
```


================
Comment at: libcxx/include/__format/formatter_integral.h:363-364
+    if (__size >= this->__width)
+      return _VSTD::transform(__first, __last, _VSTD::move(__out_it),
+                              __hex_to_upper);
+
----------------
Maybe add a TODO to format upper-case hex in one pass? As a nice side effect you could probably also kill dependency on `<algorithm>` which is super bloated in 20.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433