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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the reviews!



================
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>;
+};
----------------
Quuxplusone wrote:
> 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`.)
No that's not useful since at the moment those functions aren't `const` and can't be `const`.
When the width is formatting argument its value is resolved in `format` and modifies the object. This obviously needs to be resolved when your LWG3636 gets accepted. But even if it doesn't get accepted that change probably is useful when implementing P2286 "Formatting ranges".

So I've already investigated how I want to resolve this. However I first like to know the final resolution for LWG3576 "Clarifying fill character in std::format". When implementing its resolution it's a good time to make sure these functions can be `const`.


================
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
----------------
Quuxplusone wrote:
> Isn't it equally easy to
> ```
> #define TEST_ENABLED_AFTER_CXX23(T, CharT) disabled<std::formatter<T, CharT>>
> ```
> ?
That's also an option. For now I just want to explain why I've taken the current approach for disable. How I'm doing the final macro is for a later review. (Basically the current approach makes it easy to test for enabled based on the language Standard used.)


================
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
----------------
Quuxplusone wrote:
> 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>();`.
Guess some comment would be good ;-) It's intended and executes twice. This formatter converts a `char` to a `wchar_t`. It's a one of a kind and specified in the Standard. Since it's part of P0645 I wanted to keep it here.


================
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>();
+}
----------------
Quuxplusone wrote:
> 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.
Yes there can be a lot more tests. But I think you have a good point about Widget and it should still be disabled. I only prefer not to add them here. I think `test_disabled` is a better place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115989