[libcxx-commits] [PATCH] D103413: [libc++][format] Implement Unicode support.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:19
 #include <__variant/monostate.h>
+#include <algorithm>
+#include <bit>
----------------
vitaut wrote:
> I wonder if it is possible to pull in a subset of `<algorithm>` because the whole thing is enormous, especially in C++20?
I think that's now possible since the header has been split recently. I'll have to see whether this project has been completed or still work in progress.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:660-662
+   * number of columns in the output. So if the formatted output has only a
+   * minimum width the exact size isn't important. It's only important to know
+   * the minimum has been reached.
----------------
vitaut wrote:
> I am not sure I understand what "the formatted output has only a minimum width" means. What is the minimum width?
The minimum width is the width specified in the format-spec. For example `std::format("{:10}", MyString);` As soon as the code has determined `MyString` results in at least 10 output columns it knows no padding is required. At that point it's a waste of CPU cycles to determine the exact number of output columns `MyString` will result to. So at that point the algorithm stops prematurely.

I think I'll add some more comment regarding this behavior.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:668
+   * * @ref __align == @c false the @ref __size is the estimated number of
+   *   columns required or 0 when the estimation algorithm stopped prematurely.
+   */
----------------
vitaut wrote:
> Why would the estimation algorithm stop prematurely?
As described in the comment above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103413