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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 14 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__format/parser_std_format_spec.h:336
 
-protected:
   /**
----------------
EricWF wrote:
> Why comment it out?
For the constexpr version the code needs some of these internals . For prototypes I love to make everything public. In the next revision I've restored protected and only made the required parts public.


================
Comment at: libcxx/include/format:231
+
+  _LIBCPP_HIDE_FROM_ABI constexpr __compile_time_handle()
+      : __parse_([](basic_format_parse_context<_CharT>&) { __throw_format_error("Not a handle"); }) {}
----------------
EricWF wrote:
> Isn't it undefined behavior to have a constexpr function that is never constexpr?
The constructor is `constexpr`, the `__parse_` won't be when called. The code should first set the proper handler. I've added comment to this constructor.


================
Comment at: libcxx/include/format:235
+private:
+  void (*__parse_)(basic_format_parse_context<_CharT>&);
+};
----------------
EricWF wrote:
> I have a weak preference for using an alias to define the type  to simplify the declaration
Since the typedef would only be used once I prefer to keep it as is.


================
Comment at: libcxx/include/format:333
+
+  if constexpr (_HasPrecision)
+    if (__formatter.__precision_needs_substitution())
----------------
EricWF wrote:
> Why does this need to be if constexpr? 
> It doesn't look like it instantiates anything 
Not every `__formatter` type has a `__precision_needs_substitution()` member function. So this avoids ill-formed code when the formatter doesn't have this member. Note that every formatter has a `__width_needs_substitution`.


================
Comment at: libcxx/include/format:490
+  requires convertible_to<const _Tp&, basic_string_view<_CharT>>
+  consteval __basic_format_string(const _Tp& __str) : __str_{__str} {
+    __format::__vformat_to(basic_format_parse_context<_CharT>{__str_, sizeof...(_Args)},
----------------
EricWF wrote:
> Why not just take a string view as the parameter? The overload won't be selected unless the argument can be implicitly converted to string view. 
I agree, however this matches the wording in the Standard http://eel.is/c++draft/format.fmt.string
In general I prefer to follow the wording of the Standard.


================
Comment at: libcxx/include/format:510
+template <class... _Args>
+using __wformat_string = __basic_format_string<wchar_t, type_identity_t<_Args>...>;
+#endif
----------------
EricWF wrote:
> Can you do the type identity thing once on the top level type rather than a bunch on each of the arguments? 
This `using` matches the wording in the Standard http://eel.is/c++draft/format.syn.


================
Comment at: libcxx/include/format:546
 template <output_iterator<const char&> _OutIt, class... _Args>
 _LIBCPP_ALWAYS_INLINE _LIBCPP_HIDE_FROM_ABI _LIBCPP_AVAILABILITY_FORMAT _OutIt
+format_to(_OutIt __out_it, __format_string<_Args...> __fmt, const _Args&... __args) {
----------------
EricWF wrote:
> Why are we always in lining this?
This is a temporary solution to improve the debug code generation.
FYI The formatting code is still work in progress. It's quite large and so some improvements will be done later. However we'll not mark this header API/ABI stable until we're happy with it.


================
Comment at: libcxx/include/format:548
+format_to(_OutIt __out_it, __format_string<_Args...> __fmt, const _Args&... __args) {
+  return _VSTD::vformat_to(_VSTD::move(__out_it), __fmt.__str_,
                            _VSTD::make_format_args(__args...));
----------------
EricWF wrote:
> Is that move required by the standard? Moving an iterator seems weird. 
It's required. The `output_iterator` concept only requires movable. 
See https://cplusplus.github.io/LWG/issue3539 for more details.


================
Comment at: libcxx/test/std/utilities/format/format.functions/format.pass.cpp:41
 
-auto test_exception = []<class CharT, class... Args>(std::string_view what, std::basic_string_view<CharT> fmt,
-                                                     const Args&... args) {
-#ifndef TEST_HAS_NO_EXCEPTIONS
-  try {
-    std::format(fmt, args...);
-#  ifndef TEST_HAS_NO_LOCALIZATION
-    if constexpr (std::same_as<CharT, char>)
-      std::cerr << "\nFormat string   " << fmt << "\nDidn't throw an exception.\n";
-#  endif
-    assert(false);
-  } catch (const std::format_error& e) {
-    if constexpr (std::same_as<CharT, char>)
-#  if defined(_LIBCPP_VERSION) && !defined(TEST_HAS_NO_LOCALIZATION)
-      if (e.what() != what)
-        std::cerr << "\nFormat string   " << fmt << "\nExpected exception " << what << "\nActual exception   "
-                  << e.what() << '\n';
-#  endif
-    LIBCPP_ASSERT(e.what() == what);
-    return;
-  }
-  assert(false);
+#define check(e, fmt, ...)                                                                                             \
+  {                                                                                                                    \
----------------
EricWF wrote:
> Please don't write giant macro test like this.
> 
> Macros are hard for readers. They're hard for tooling, they're hard for IDEs. Their diagnostics are awful.
> 
> Repetitive code in tests is fine. What's important is that it be correct upon inspection. We don't have tests for our tests. 
> 
> Optimized tests for readability. That will get you correctness. 
> 
> 
> 
> 
As discussed on Discord this has been improved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121530