[libcxx-commits] [PATCH] D96664: [libc++][format] Implement formatters.

[Louis Dionne via Phabricator via libcxx-commits]

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

Got a few comments on the tests, but apart from that this mostly LGTM. I'm assuming that the several TODOs are handled in subsequent patches.



================
Comment at: libcxx/include/__format/formatter.h:51
+private:
+  _LIBCPP_NORETURN _LIBCPP_HIDDEN void __throw() {
+    __throw_format_error("Argument type not implemented yet");
----------------
Why is this `_LIBCPP_HIDDEN` and not `_LIBCPP_HIDE_FROM_ABI`?


================
Comment at: libcxx/test/std/utilities/format/format.functions/format_to_n.pass.cpp:11
+// UNSUPPORTED: libcpp-has-no-incomplete-format
+// UNSUPPORTED: gcc-10, gcc-11
+
----------------
You can remove mentions of `gcc-10` everywhere now.

Also, it would be great to have a comment explaining why this is unsupported on `gcc-11` (in all places where you do that).


================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:18
+template <class CharT>
+void test() {
+  // *** Test escaping  ***
----------------
Instead of using this inclusion technique, I would rather see this done this way:

```
// TestFunction must be callable as check(expected-result, string-to-format, args-to-format...)
// ExceptionTest must be callable as check_exception(expected-exception, string-to-format, args-to-format...)
template <class CharT, class TestFunction, class ExceptionTest>
void format_tests(TestFunction check, ExceptionTest check_exception) {
  check(STR("hello 01"), STR("hello {0:}{1:}"), false, true);
  // ...
}
```

Then, from the various tests, you can do:

```
#include "format_tests.h"

auto test = []<class CharT, class... Args>(std::basic_string<CharT> expected, std::basic_string<CharT> fmt, const Args&... args) {
  // ...
};

auto test_exception = []<class CharT, class... Args>(std::string_view what, std::basic_string<CharT> fmt, const Args&... args) {
  // ...
};

int main(int, char**) {
  format_tests<char>(test, test_exception);
  format_tests<wchar_t>(test, test_exception);

  return 0;
}
```

IMO this is a less surprising way to reuse code and that makes it easier to understand what's happening when you look at the test in isolation. WDYT?


================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat.locale.pass.cpp:42
+                    const Args&... args) {
+#ifndef _LIBCPP_NO_EXCEPTIONS
+  try {
----------------
This should be `TEST_HAS_NO_EXCEPTIONS` (elsewhere too).


================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat.pass.cpp:24
+
+// Helper functions for this specific test.
+
----------------
Those comments are not especially useful IMO, we could remove them altogether.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664