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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review.



================
Comment at: libcxx/include/__format/formatter_pointer.h:41
+
+namespace __format_spec {
+
----------------
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.


================
Comment at: libcxx/include/__format/formatter_pointer.h:56
+    // TODO FMT Remove code duplication.
+    char __buffer[2 + sizeof(uintptr_t)];
+    __buffer[0] = '0';
----------------
vitaut wrote:
> I think the buffer size should be `2 + 2 * sizeof(uintptr_t)`. Note extra factor of two because each byte may results in two hex characters.
Good catch, seems there's a test missing ;-)


================
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));
----------------
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. 


================
Comment at: libcxx/test/std/utilities/format/format.functions/format_tests.h:2510-2512
+  format_test_pointer<std::nullptr_t, CharT>(check, check_exception);
+  format_test_pointer<void*, CharT>(check, check_exception);
+  format_test_pointer<const void*, CharT>(check, check_exception);
----------------
vitaut wrote:
> Do you test anywhere that non-void pointers are disallowed?
At the moment they are allowed, this is addressed in D115989.
(See also https://reviews.llvm.org/D115988#inline-1108939)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115988