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

[Louis Dionne via Phabricator via libcxx-commits]

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

Nothing major, I'm leaving the correctness of the implementation to @vitaut, who already reviewed it.



================
Comment at: libcxx/include/__format/parser_std_format_spec.h:27-28
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
----------------
You don't need this (nor the `_LIBCPP_POP_MACROS` at the end) if you don't use `min()` or `max()` in this file.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:141
+  __parse(const _CharT* __begin, const _CharT* __end, _Flags& __flags) {
+    _LIBCPP_ASSERT(__begin != __end, "Precondition failure");
+    if (__begin + 1 != __end) {
----------------
This assertion message is not very helpful, we should explain *why* it's unexpected that `begin == end` instead.


================
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()) {
----------------



================
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
----------------



================
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.
+
----------------
What do you mean by "freestanding" here?

Also, this header should include what it uses.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103368