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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
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.
+ */
----------------
vitaut wrote:
> 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.
Valid point and I think the 128-bit work-around is the only place it's used.


================
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");
----------------
vitaut wrote:
> Why not use a conditional expression?
> 
> ```
>         return __format_unsigned_integral(__array.begin(), __array.end(),
>                                           __value, __negative, 8, __ctx, __value != 0 ? "0" : nullptr);
> ```
No specific reason not to use it. But in this case looks better with the conditional expression.


================
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);
+
----------------
vitaut wrote:
> 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.
I'll add a TODO. I'm not sure I'll be able to remove `<algorithm>`, I use more parts of it. But I can have a look to use the new smaller `<__algorithm/foo.h>` headers. That's something that has been worked on after my initial version of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103433