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

[Tom Honermann via Phabricator via libcxx-commits]

tahonermann added a comment.

This looks great. I added one comment seeking clarification when an encoding error is present. If the code is as intended, it might help to add a comment to explain what is happening.



================
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");
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144742