From b7a4c4482fdb839ba767d5125aed6e35e34276d1 Mon Sep 17 00:00:00 2001 From: asynts Date: Wed, 23 Sep 2020 13:21:18 +0200 Subject: [PATCH] AK: Resolve format related circular dependencies properly. With this commit, has a more supportive role and isn't used directly. Essentially, there now is a public 'vformat' function ('v' for vector) which takes already type erased parameters. The name is choosen to indicate that this function behaves similar to C-style functions taking a va_list equivalent. The interface for frontend users are now 'String::formatted' and 'StringBuilder::appendff'. --- AK/Format.cpp | 40 ++++++++++++++---------------- AK/Format.h | 48 ++++++++++++------------------------ AK/LogStream.cpp | 1 + AK/String.cpp | 34 ++++++++++++++++++++++--- AK/String.h | 34 +++++++++---------------- AK/StringBuilder.h | 8 ++++-- AK/Tests/TestFormat.cpp | 39 +++++++++++++++-------------- Libraries/LibGUI/Clipboard.h | 1 + 8 files changed, 105 insertions(+), 100 deletions(-) diff --git a/AK/Format.cpp b/AK/Format.cpp index aad9a12dab..9868a559b2 100644 --- a/AK/Format.cpp +++ b/AK/Format.cpp @@ -27,9 +27,10 @@ #include #include #include +#include #include -namespace AK::Detail::Format { +namespace { struct FormatSpecifier { StringView flags; @@ -102,14 +103,11 @@ static bool parse_format_specifier(StringView input, FormatSpecifier& specifier) return true; } -String format(StringView fmtstr, AK::Span formatters, size_t argument_index) -{ - StringBuilder builder; - format(builder, fmtstr, formatters, argument_index); - return builder.to_string(); -} +} // namespace -void format(StringBuilder& builder, StringView fmtstr, AK::Span formatters, size_t argument_index) +namespace AK { + +void vformat(StringBuilder& builder, StringView fmtstr, AK::Span parameters, size_t argument_index) { size_t opening; if (!find_next_unescaped(opening, fmtstr, '{')) { @@ -135,19 +133,24 @@ void format(StringBuilder& builder, StringView fmtstr, AK::Span::max()) specifier.index = argument_index++; - if (specifier.index >= formatters.size()) + if (specifier.index >= parameters.size()) ASSERT_NOT_REACHED(); - auto& formatter = formatters[specifier.index]; - if (!formatter.format(builder, formatter.parameter, specifier.flags)) + auto& parameter = parameters[specifier.index]; + if (!parameter.formatter(builder, parameter.value, specifier.flags)) ASSERT_NOT_REACHED(); - format(builder, fmtstr.substring_view(closing + 1), formatters, argument_index); + vformat(builder, fmtstr.substring_view(closing + 1), parameters, argument_index); } -} // namespace AK::Detail::Format - -namespace AK { +bool Formatter::parse(StringView flags) +{ + return flags.is_empty(); +} +void Formatter::format(StringBuilder& builder, StringView value) +{ + builder.append(value); +} template bool Formatter::value>::Type>::parse(StringView flags) @@ -159,14 +162,13 @@ bool Formatter::value>::Type>::parse(StringVi auto field_width = lexer.consume_while([](char ch) { return StringView { "0123456789" }.contains(ch); }); if (field_width.length() > 0) - this->field_width = Detail::Format::parse_number(field_width); + this->field_width = parse_number(field_width); if (lexer.consume_specific('x')) hexadecimal = true; return lexer.is_eof(); } - template void Formatter::value>::Type>::format(StringBuilder& builder, T value) { @@ -180,8 +182,6 @@ void Formatter::value>::Type>::format(StringB PrintfImplementation::print_i64([&](auto, char ch) { builder.append(ch); }, bufptr, value, false, zero_pad, field_width); } -template struct Formatter; -template struct Formatter; template struct Formatter; template struct Formatter; template struct Formatter; @@ -192,8 +192,6 @@ template struct Formatter; template struct Formatter; template struct Formatter; template struct Formatter; - -// C++ is weird. template struct Formatter; } // namespace AK diff --git a/AK/Format.h b/AK/Format.h index 03a0cb4cd6..0d9ba16349 100644 --- a/AK/Format.h +++ b/AK/Format.h @@ -27,9 +27,12 @@ #pragma once #include -#include #include +// FIXME: I would really love to merge the format_value and make_type_erased_parameters functions, +// but the compiler creates weird error messages when I do that. Here is a small snippet that +// reproduces the issue: https://godbolt.org/z/o55crs + namespace AK { template @@ -51,36 +54,25 @@ bool format_value(StringBuilder& builder, const void* value, StringView flags) return true; } -struct TypeErasedFormatter { - bool (*format)(StringBuilder& builder, const void* value, StringView flags); - const void* parameter; -}; - -template -TypeErasedFormatter make_type_erased_formatter(const T& value) { return { format_value, &value }; } - -String format(StringView fmtstr, AK::Span, size_t argument_index = 0); -void format(StringBuilder&, StringView fmtstr, AK::Span, size_t argument_index = 0); - } // namespace AK::Detail::Format namespace AK { -template -struct Formatter { - bool parse(StringView) { return true; } - void format(StringBuilder& builder, const char* value) { builder.append(value); } +struct TypeErasedParameter { + const void* value; + bool (*formatter)(StringBuilder& builder, const void* value, StringView flags); }; template<> struct Formatter { - bool parse(StringView flags) { return flags.is_empty(); } - void format(StringBuilder& builder, StringView value) { builder.append(value); } + bool parse(StringView flags); + void format(StringBuilder& builder, StringView value); +}; +template +struct Formatter : Formatter { }; template<> -struct Formatter { - bool parse(StringView flags) { return flags.is_empty(); } - void format(StringBuilder& builder, const String& value) { builder.append(value); } +struct Formatter : Formatter { }; template @@ -94,19 +86,11 @@ struct Formatter::value>::Type> { }; template -String format(StringView fmtstr, const Parameters&... parameters) +Array make_type_erased_parameters(const Parameters&... parameters) { - Array formatters { Detail::Format::make_type_erased_formatter(parameters)... }; - return Detail::Format::format(fmtstr, formatters); -} -template -void format(StringBuilder& builder, StringView fmtstr, const Parameters&... parameters) -{ - Array formatters { Detail::Format::make_type_erased_formatter(parameters)... }; - Detail::Format::format(builder, fmtstr, formatters); + return { TypeErasedParameter { ¶meters, Detail::Format::format_value }... }; } -template -void StringBuilder::appendff(StringView fmtstr, const Parameters&... parameters) { AK::format(*this, fmtstr, parameters...); } +void vformat(StringBuilder& builder, StringView fmtstr, Span, size_t argument_index = 0); } // namespace AK diff --git a/AK/LogStream.cpp b/AK/LogStream.cpp index 8bcbe6ffa7..37a7a58e08 100644 --- a/AK/LogStream.cpp +++ b/AK/LogStream.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #ifdef KERNEL diff --git a/AK/String.cpp b/AK/String.cpp index 29a5915419..57582ca22e 100644 --- a/AK/String.cpp +++ b/AK/String.cpp @@ -216,7 +216,7 @@ Optional String::to_uint() const } template -String String::number(T value) { return AK::format("{}", value); } +String String::number(T value) { return formatted("{}", value); } template String String::number(unsigned char); template String String::number(unsigned short); @@ -228,8 +228,6 @@ template String String::number(short); template String String::number(int); template String String::number(long); template String String::number(long long); - -// C++ is weird. template String String::number(signed char); String String::format(const char* fmt, ...) @@ -458,4 +456,34 @@ StringView String::view() const return { characters(), length() }; } +InputStream& operator>>(InputStream& stream, String& string) +{ + StringBuilder builder; + + for (;;) { + char next_char; + stream >> next_char; + + if (stream.has_any_error()) { + stream.set_fatal_error(); + string = nullptr; + return stream; + } + + if (next_char) { + builder.append(next_char); + } else { + string = builder.to_string(); + return stream; + } + } +} + +String String::vformatted(StringView fmtstr, Span parameters) +{ + StringBuilder builder; + vformat(builder, fmtstr, parameters); + return builder.to_string(); +} + } diff --git a/AK/String.h b/AK/String.h index 6575231638..3c36d84b17 100644 --- a/AK/String.h +++ b/AK/String.h @@ -26,10 +26,10 @@ #pragma once +#include #include #include #include -#include #include #include #include @@ -239,6 +239,15 @@ public: static String format(const char*, ...); + static String vformatted(StringView fmtstr, Span); + + template + static String formatted(StringView fmtstr, const Parameters&... parameters) + { + const auto type_erased_parameters = make_type_erased_parameters(parameters...); + return vformatted(fmtstr, type_erased_parameters); + } + template static String number(T); @@ -277,28 +286,7 @@ bool operator<=(const char*, const String&); String escape_html_entities(const StringView& html); -inline InputStream& operator>>(InputStream& stream, String& string) -{ - StringBuilder builder; - - for (;;) { - char next_char; - stream >> next_char; - - if (stream.has_any_error()) { - stream.set_fatal_error(); - string = nullptr; - return stream; - } - - if (next_char) { - builder.append(next_char); - } else { - string = builder.to_string(); - return stream; - } - } -} +InputStream& operator>>(InputStream& stream, String& string); } diff --git a/AK/StringBuilder.h b/AK/StringBuilder.h index cd67475cb2..54515a81bd 100644 --- a/AK/StringBuilder.h +++ b/AK/StringBuilder.h @@ -27,6 +27,7 @@ #pragma once #include +#include #include #include #include @@ -48,9 +49,12 @@ public: void appendf(const char*, ...); void appendvf(const char*, va_list); - // Implemented in to break circular dependency. template - void appendff(StringView fmtstr, const Parameters&...); + void appendff(StringView fmtstr, const Parameters&... parameters) + { + const auto type_erased_parameters = make_type_erased_parameters(parameters...); + vformat(*this, fmtstr, type_erased_parameters); + } String build() const; String to_string() const; diff --git a/AK/Tests/TestFormat.cpp b/AK/Tests/TestFormat.cpp index 505b13bf13..a58e82850a 100644 --- a/AK/Tests/TestFormat.cpp +++ b/AK/Tests/TestFormat.cpp @@ -26,43 +26,44 @@ #include -#include +#include +#include TEST_CASE(format_string_literals) { - EXPECT_EQ(AK::format("prefix-{}-suffix", "abc"), "prefix-abc-suffix"); - EXPECT_EQ(AK::format("{}{}{}", "a", "b", "c"), "abc"); + EXPECT_EQ(String::formatted("prefix-{}-suffix", "abc"), "prefix-abc-suffix"); + EXPECT_EQ(String::formatted("{}{}{}", "a", "b", "c"), "abc"); } TEST_CASE(format_integers) { - EXPECT_EQ(AK::format("{}", 42u), "42"); - EXPECT_EQ(AK::format("{:4}", 42u), " 42"); - EXPECT_EQ(AK::format("{:08}", 42u), "00000042"); - // EXPECT_EQ(AK::format("{:7}", -17), " -17"); - EXPECT_EQ(AK::format("{}", -17), "-17"); - EXPECT_EQ(AK::format("{:04}", 13), "0013"); - EXPECT_EQ(AK::format("{:08x}", 4096), "00001000"); - EXPECT_EQ(AK::format("{:x}", 0x1111222233334444ull), "1111222233334444"); + EXPECT_EQ(String::formatted("{}", 42u), "42"); + EXPECT_EQ(String::formatted("{:4}", 42u), " 42"); + EXPECT_EQ(String::formatted("{:08}", 42u), "00000042"); + // EXPECT_EQ(String::formatted("{:7}", -17), " -17"); + EXPECT_EQ(String::formatted("{}", -17), "-17"); + EXPECT_EQ(String::formatted("{:04}", 13), "0013"); + EXPECT_EQ(String::formatted("{:08x}", 4096), "00001000"); + EXPECT_EQ(String::formatted("{:x}", 0x1111222233334444ull), "1111222233334444"); } TEST_CASE(reorder_format_arguments) { - EXPECT_EQ(AK::format("{1}{0}", "a", "b"), "ba"); - EXPECT_EQ(AK::format("{0}{1}", "a", "b"), "ab"); - EXPECT_EQ(AK::format("{0}{0}{0}", "a", "b"), "aaa"); - EXPECT_EQ(AK::format("{1}{}{0}", "a", "b", "c"), "baa"); + EXPECT_EQ(String::formatted("{1}{0}", "a", "b"), "ba"); + EXPECT_EQ(String::formatted("{0}{1}", "a", "b"), "ab"); + EXPECT_EQ(String::formatted("{0}{0}{0}", "a", "b"), "aaa"); + EXPECT_EQ(String::formatted("{1}{}{0}", "a", "b", "c"), "baa"); } TEST_CASE(escape_braces) { - EXPECT_EQ(AK::format("{{{}", "foo"), "{foo"); - EXPECT_EQ(AK::format("{}}}", "bar"), "bar}"); + EXPECT_EQ(String::formatted("{{{}", "foo"), "{foo"); + EXPECT_EQ(String::formatted("{}}}", "bar"), "bar}"); } TEST_CASE(everything) { - EXPECT_EQ(AK::format("{{{:04}/{}/{0:8}/{1}", 42u, "foo"), "{0042/foo/ 42/foo"); + EXPECT_EQ(String::formatted("{{{:04}/{}/{0:8}/{1}", 42u, "foo"), "{0042/foo/ 42/foo"); } TEST_CASE(string_builder) @@ -76,7 +77,7 @@ TEST_CASE(string_builder) TEST_CASE(format_without_arguments) { - EXPECT_EQ(AK::format("foo"), "foo"); + EXPECT_EQ(String::formatted("foo"), "foo"); } TEST_MAIN(Format) diff --git a/Libraries/LibGUI/Clipboard.h b/Libraries/LibGUI/Clipboard.h index e5830268ea..a01f96e509 100644 --- a/Libraries/LibGUI/Clipboard.h +++ b/Libraries/LibGUI/Clipboard.h @@ -26,6 +26,7 @@ #pragma once +#include #include #include #include