[libcxx-commits] [PATCH] D115988: [libc++][format] Adds formatter pointer.

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut added inline comments.


================
Comment at: libcxx/include/__format/formatter_pointer.h:41
+
+namespace __format_spec {
+
----------------
Mordante wrote:
> vitaut wrote:
> > What is this namespace for? `__formatter_<foo>` is already very specific.
> This is consistent with the other formatters so I prefer to keep it that way.
> But maybe I'll change it, but than for all code in these namespaces.
Yeah, I'd recommend dropping this namespace in a separate diff to keep symbol names manageable and since the names already have `__format...` in them.


================
Comment at: libcxx/test/std/utilities/format/format.functions/format_tests.h:2487
+  // *** Sign ***
+  check_exception("The format-spec should consume the input or end with a '}'", STR("{:-}"), P(nullptr));
+  check_exception("The format-spec should consume the input or end with a '}'", STR("{:+}"), P(nullptr));
----------------
Mordante wrote:
> vitaut wrote:
> > nit: I think the error message is a bit too technical. Something simpler along the lines of "Invalid format specifier" might be more user-friendly.
> This message is consistent with the message of the other parsers/formatters.
> Do you suggest to change all messages to only contain "Invalid format specifier" ?
> 
> I will keep it as is in this patch. I wouldn't mind to change it, but in a separate patch and addressing all messages to keep them consistent. 
> Do you suggest to change all messages to only contain "Invalid format specifier" ?

Yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115988