[libcxx-commits] [PATCH] D121530: [libc++][format] Implement format-string.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

This LGTM. I think most comments by @vitaut have been applied, and while I have a couple of comments, I think this is good to go.



================
Comment at: libcxx/docs/ReleaseNotes.rst:116
+- The format functions (``std::format``, ``std::format_to``,
+  ``std::format_to_n``, and ``std::formatted_size``) now validate the format
+  string at compile time. This means the argument needs to be a compile-time
----------------
I think we should move these "API Changes" to "New features" above, but without mentioning that this can break code. Indeed, these release notes are aimed at LLVM releases, however `<format>` has not been enabled yet in LLVM releases, so calling this an API change is going to be misleading. I think we should only explain that format strings are now checked at compile-time.


================
Comment at: libcxx/include/format:488-490
+    array<__arg_t, sizeof...(_Args)> __result;
+    __arg_t* __types = __result.data();
+    ([&] { *__types++ = __determine_arg_t<_Context, _Args>(); }(), ...);
----------------
Would this work too? If so, it may even become unnecessary to have a `__get_types` function altogether, perhaps we can just inline it in the body of `__basic_format_string` below.


================
Comment at: libcxx/test/std/utilities/format/format.functions/format.locale.pass.cpp:45
+  // Therefore this tests does nothing.
+  // A basic ill-formed test is done in format.localeverify.cpp
+  // The exceptions are tested by other functions that don't use the basic-format-string as fmt argument.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530