[libcxx-commits] [PATCH] D116381: [libc++][format] Fix precision parser conformance.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for the reviews!

In D116381#3213091 <https://reviews.llvm.org/D116381#3213091>, @CaseyCarter wrote:

>> @CaseyCarter reported the std-format-spec parser prohibits leading zeros
>
> Technically, I reported that the _test_ expects the std-format-spec parser to reject leading zeroes for precision, which the Working Draft does not require.  I wouldn't write this silly format-spec even if it is allowed, but I did notice that MSVC STL fails this test.

True, I can reword the message. But without your report this bug wouldn't be fixed ;-) And I agree these tests are probably the only time I'll write the leading zeros for precision; unless I want to do a C++ Pub quiz.



================
Comment at: libcxx/test/std/utilities/format/format.functions/format_tests.h:180
+  check_exception(
+      "A format-spec arg-id should terminate at a '}'",
+      STR("hello {0:{01}}"), world, 1);
----------------
CaseyCarter wrote:
> Quuxplusone wrote:
> > It would be friendlier to make this message `"A format-spec arg-id shouldn't have leading zeros"`.
> > Also (definitely unrelated to this PR) I think it's superfluous to keep repeating the word `format-spec` in all these messages. The exception type is already `format_error` and it's thrown during format-string parsing, right? I think `"An arg-id shouldn't...` would be perfectly unambiguous to the catcher.
> +1 for "... shouldn't have leading zeroes". A user who mistakenly believes that `01` is a valid _arg-id_ will read this message, see all the `}`s in the format string, and spend an hour investigating the wrong thing / filing a bug report when we could have pointed them directly at the problem.
@vitaut suggested here https://reviews.llvm.org/D115988#inline-1110797 to simplify all messages. So for now I keep this message as is. If we keep multiple errors it should indeed be a better message, but if we're going to simplify them I rather keep this one as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116381