[libcxx-commits] [PATCH] D144742: [libc++][format] Improves fill character.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:451-456
+    __unicode::__consume_result __consumed = __view.__consume();
+    ptrdiff_t __code_units                 = __view.__position() - __begin;
+    if (__view.__position() < __end) {
+      if (__parse_alignment(*__view.__position())) {
+        if (__consumed.__status != __unicode::__consume_result::__ok)
+          std::__throw_format_error("The fill character contains an invalid value");
----------------
tahonermann wrote:
> This seems a little odd to me. When consumption of the fill character fails (due to an encoding issue), an attempt is still made to parse the alignment at the new position before checking for the consumption error and then reporting a parse issue. I'm not sure why that attempt is made since success is going to lead to reporting the fill character failure and failure is going to result in falling through to retry parsing the alignment at the beginning anyway. If the intent is to get to different error messages, that seems reasonable, but it seems this can fallback to trying to parse the alignment at the beginning anyway. Am I missing something?
I wrote this in this way due to historic reasons. A character like 0 is a fill-character when followed by an alignment. Else it's a zero-padding. Since valid elements of the format-spec are not invalid Unicode this is not needed. I adjusted it, but reverted it again. For UTF-32 it makes sense to only test when it's a fill character, and I like to keep the same diagnostic regardless of the encoding used. So I added comment to explain the design.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144742