[libcxx-commits] [PATCH] D121530: [WIP][libc++][format] Implement format-string.
Eric Fiselier via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Mar 12 15:30:04 PST 2022
EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.
The patch to the headers looks phenomenal. I only reviewed the code. I haven't looked at the spec yet. But the code was gorgeous. Well done.
The test needs to be entirely rewritten. The macro stuff is complicated and hard to read. Tests need to be correct upon inspection. That means repeating yourself rather than adding in direction.
Also that test file is way too big. Come up with a nice way to split it into different files.
Thank you for working on this.
================
Comment at: libcxx/include/__format/parser_std_format_spec.h:336
-protected:
/**
----------------
Why comment it out?
================
Comment at: libcxx/include/format:88
format_to_n_result<Out> format_to_n(Out out, iter_difference_t<Out> n,
- const locale& loc, wstring_view fmt,
+ const locale& loc, wformat-string<Args...> fmt,
const Args&... args);
----------------
There's some delicious irony in that name.
================
Comment at: libcxx/include/format:216
namespace __format {
+template <class _CharT>
----------------
I think you've done a really good job of minimizing the number of instantiations required to do all this compile time work. Well done!
================
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"); }) {}
----------------
Isn't it undefined behavior to have a constexpr function that is never constexpr?
================
Comment at: libcxx/include/format:235
+private:
+ void (*__parse_)(basic_format_parse_context<_CharT>&);
+};
----------------
I have a weak preference for using an alias to define the type to simplify the declaration
================
Comment at: libcxx/include/format:298
+_LIBCPP_HIDE_FROM_ABI
+constexpr void __compile_time_validate_integral(__arg_t __type) {
+ switch (__type) {
----------------
This is beautiful
================
Comment at: libcxx/include/format:333
+
+ if constexpr (_HasPrecision)
+ if (__formatter.__precision_needs_substitution())
----------------
Why does this need to be if constexpr?
It doesn't look like it instantiates anything
================
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)},
----------------
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.
================
Comment at: libcxx/include/format:510
+template <class... _Args>
+using __wformat_string = __basic_format_string<wchar_t, type_identity_t<_Args>...>;
+#endif
----------------
Can you do the type identity thing once on the top level type rather than a bunch on each of the arguments?
================
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) {
----------------
Why are we always in lining this?
================
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...));
----------------
Is that move required by the standard? Moving an iterator seems weird.
================
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, ...) \
+ { \
----------------
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.
================
Comment at: libcxx/test/std/utilities/format/format.functions/format_tests.h:33
+# define CALL_CHECK_FUNCTION(f) f()
+# define check_ill_formed(what, fmt, ...) \
+ static_assert([]<class = void>() { \
----------------
No. Absolutely not.
We need to find a simpler way to write this in normal code. They can be reused reasonably. But this is way too complex to be at the root of correct tests. By the time these macros are used, who can have any idea what's going on.
I apologize for being terse
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121530/new/
https://reviews.llvm.org/D121530
More information about the libcxx-commits
mailing list