[libcxx-commits] [PATCH] D115989: [libc++][format] Disable default formatter.

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sat Dec 18 09:57:05 PST 2021


Quuxplusone added a comment.

Test nits. I assume the code is correct.



================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp:67-68
+    requires(F f, std::basic_format_parse_context<CharT> pc, T u, std::basic_format_context<CharT*, CharT> fc) {
+  { f.parse(pc) } -> std::same_as<typename decltype(pc)::iterator>;
+  { f.format(u, fc) } -> std::same_as<typename decltype(fc)::iterator>;
+};
----------------
Would it be useful to verify here that `std::as_const(f).format(u, fc)` is also OK?
(Recall our earlier discussion of how standard formatters should be const-friendly because eventually they'll be required to be constexpr-friendly. This could be tucked behind an `#ifdef _LIBCPP_VER`.)


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp:81
+// #else
+//   TEST_ENABLED_AFTER_CXX23(T, CharT) disabled<T, CharT>
+// #endif
----------------
Isn't it equally easy to
```
#define TEST_ENABLED_AFTER_CXX23(T, CharT) disabled<std::formatter<T, CharT>>
```
?


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp:112-114
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  assert_formatter_is_enabled<char, wchar_t>();
+#endif
----------------
This (non-template-dependent) test seems out of place, but whatever.
As long as it's not a typo for `assert_formatter_is_enabled<CharT, wchar_t>();`.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp:249
+  assert_formatter_is_disabled<std::pair<int, int>, CharT>();
+  assert_formatter_is_disabled<std::tuple<int>, CharT>();
+}
----------------
For all of these, once the `int` versions are enabled, I assume you'll also need to check that the e.g. `struct Widget` versions are still disabled:
```
assert_formatter_is_disabled<std::array<Widget, 42>, CharT>();
```
etc.


================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.formatter.spec/types.compile.pass.cpp:268
+  assert_formatter_is_disabled<std::optional<int>, CharT>();
+  assert_formatter_is_disabled<std::variant<int>, CharT>();
+}
----------------
```
assert_formatter_is_disabled<const int*, CharT>();
assert_formatter_is_disabled<c, CharT>();
assert_formatter_is_disabled<c*, CharT>();
assert_formatter_is_disabled<const c*, CharT>();
assert_formatter_is_disabled<const char*, wchar_t>();
assert_formatter_is_disabled<const char*, char8_t>();  // ?
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115989



More information about the libcxx-commits mailing list