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

[Mark de Wever via Phabricator via libcxx-commits]

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

In D96664#2975281 <https://reviews.llvm.org/D96664#2975281>, @ldionne wrote:

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

Part of them will be done in the patches in this series. Others will be done in not yet finished patches. But the intention is to solve them all before marking the `<format>` header as complete.



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


================
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
+
----------------
ldionne wrote:
> 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).
I need to investigate why gcc-11 doesn't work. I've added a TODO to look at it later. I'll need to update one of my systems so I have gcc-11 locally which makes testing easier.


================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:1
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
This file is obsolete and should be removed. I prefer to keep this file a little bit longer. This will make it a lot easier to migrate the other patches in the series to the new `format_tests.h`. Once that migration is done this file can be removed.


================
Comment at: libcxx/test/std/utilities/format/format.functions/tests.inc:18
+template <class CharT>
+void test() {
+  // *** Test escaping  ***
----------------
ldionne wrote:
> 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?
I like this solution a lot! Thanks for the suggestion.


================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat.pass.cpp:24
+
+// Helper functions for this specific test.
+
----------------
ldionne wrote:
> Those comments are not especially useful IMO, we could remove them altogether.
I feel they are useful due to the usage of the `tests.inc`. But since that file will be replaced with your suggested `format_tests.h` I agree that they are not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96664