[libcxx-commits] [PATCH] D103368: [libc++][format] Adds parser std-format-spec.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 6 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:616
+   */
+  auto _LIBCPP_HIDE_FROM_ABI constexpr parse(auto& __parse_ctx)
+      -> decltype(__parse_ctx.begin()) {
----------------
ldionne wrote:
> 
Good catch, this indeed looks weird.


================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/std_format_spec_integral.pass.cpp:24
+#ifndef _LIBCPP_HAS_NO_LOCALIZATION
+#include <iostream>
+#endif
----------------
ldionne wrote:
> 
I'm not too happy with this change, it's what `clang-format` did. I created D109835 containing an updated `clang-format` settings so this won't be undone when I rerun clang-format. 


================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/test_exception.h:12
+// Helper header for the tests in this directory.
+// Note the header isn't freestanding.
+
----------------
ldionne wrote:
> What do you mean by "freestanding" here?
> 
> Also, this header should include what it uses.
Freestanding means the header depends on the state of the translation unit before the header itself. Hence it requires no includes.
It depends on the `parser` type in the test using it. I changed the unit tests to supply that argument as template argument, so the header can be a normal header.


================
Comment at: libcxx/test/libcxx/utilities/format/format.string/format.string.std/test_exception.h:22-24
+    if constexpr (std::same_as<CharT, char>)
+      std::cerr << "\nFormat string   " << fmt
+                << "\nDidn't throw an exception.\n";
----------------
ldionne wrote:
> We don't want to print error messages like that in the tests, specifically because that doesn't work on some platforms where `stderr` & friends are not supported. I'd rather just remove this `if constexpr`. Same goes for below.
I expect `stderr` to be valid on all platforms what do not define `_LIBCPP_HAS_NO_LOCALIZATION`. AFAIK this macro should be the proper guard.
I don't mind removing them too much, so I'll remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103368