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

[Victor Zverovich via Phabricator via libcxx-commits]

vitaut requested changes to this revision.
vitaut added a comment.
This revision now requires changes to proceed.

Overall looks good but I think there is a potential buffer overflow in format (see inline comment).



================
Comment at: libcxx/include/__format/formatter_pointer.h:41
+
+namespace __format_spec {
+
----------------
What is this namespace for? `__formatter_<foo>` is already very specific.


================
Comment at: libcxx/include/__format/formatter_pointer.h:52
+
+    // This code is looks a lot like the code to format a hexadecimal integral,
+    // but that code isn't public. Making that code public requires some
----------------
"is looks" -> "looks"


================
Comment at: libcxx/include/__format/formatter_pointer.h:56
+    // TODO FMT Remove code duplication.
+    char __buffer[2 + sizeof(uintptr_t)];
+    __buffer[0] = '0';
----------------
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.


================
Comment at: libcxx/include/__format/formatter_pointer.h:60
+    char* __last =
+        __to_buffer(_VSTD::addressof(__buffer[2]), _VSTD::end(__buffer), reinterpret_cast<uintptr_t>(__ptr), 16);
+
----------------
`_VSTD::addressof(__buffer[2])` can be simplified to `__buffer + 2`. `addressof` doesn't seem to be adding anything useful here.


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


================
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);
----------------
Do you test anywhere that non-void pointers are disallowed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115988