AK: Resolve format related circular dependencies properly.

With this commit, <AK/Format.h> 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

@@ -27,9 +27,10 @@
 #include <AK/Format.h>
 #include <AK/GenericLexer.h>
 #include <AK/PrintfImplementation.h>
+#include <AK/String.h>
 #include <AK/StringBuilder.h>
 
-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<TypeErasedFormatter> formatters, size_t argument_index)
+} // namespace
-{
-    StringBuilder builder;
-    format(builder, fmtstr, formatters, argument_index);
-    return builder.to_string();
-}
 
-void format(StringBuilder& builder, StringView fmtstr, AK::Span<TypeErasedFormatter> formatters, size_t argument_index)
+namespace AK {
+
+void vformat(StringBuilder& builder, StringView fmtstr, AK::Span<const TypeErasedParameter> 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<TypeErasedFormat
     if (specifier.index == NumericLimits<size_t>::max())
         specifier.index = argument_index++;
 
-    if (specifier.index >= formatters.size())
+    if (specifier.index >= parameters.size())
         ASSERT_NOT_REACHED();
 
-    auto& formatter = formatters[specifier.index];
+    auto& parameter = parameters[specifier.index];
-    if (!formatter.format(builder, formatter.parameter, specifier.flags))
+    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
+bool Formatter<StringView>::parse(StringView flags)
-
+{
-namespace AK {
+    return flags.is_empty();
+}
+void Formatter<StringView>::format(StringBuilder& builder, StringView value)
+{
+    builder.append(value);
+}
 
 template<typename T>
 bool Formatter<T, typename EnableIf<IsIntegral<T>::value>::Type>::parse(StringView flags)
@@ -159,14 +162,13 @@ bool Formatter<T, typename EnableIf<IsIntegral<T>::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<typename T>
 void Formatter<T, typename EnableIf<IsIntegral<T>::value>::Type>::format(StringBuilder& builder, T value)
 {
@@ -180,8 +182,6 @@ void Formatter<T, typename EnableIf<IsIntegral<T>::value>::Type>::format(StringB
         PrintfImplementation::print_i64([&](auto, char ch) { builder.append(ch); }, bufptr, value, false, zero_pad, field_width);
 }
 
-template struct Formatter<StringView>;
-template struct Formatter<String>;
 template struct Formatter<unsigned char, void>;
 template struct Formatter<unsigned short, void>;
 template struct Formatter<unsigned int, void>;
@@ -192,8 +192,6 @@ template struct Formatter<short, void>;
 template struct Formatter<int, void>;
 template struct Formatter<long, void>;
 template struct Formatter<long long, void>;
-
-// C++ is weird.
 template struct Formatter<signed char, void>;
 
 } // namespace AK

AK/Format.h

@@ -27,9 +27,12 @@
 #pragma once
 
 #include <AK/Array.h>
-#include <AK/String.h>
 #include <AK/StringView.h>
 
+// 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<typename T, typename = void>
@@ -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<typename T>
-TypeErasedFormatter make_type_erased_formatter(const T& value) { return { format_value<T>, &value }; }
-
-String format(StringView fmtstr, AK::Span<TypeErasedFormatter>, size_t argument_index = 0);
-void format(StringBuilder&, StringView fmtstr, AK::Span<TypeErasedFormatter>, size_t argument_index = 0);
-
 } // namespace AK::Detail::Format
 
 namespace AK {
 
-template<size_t Size>
+struct TypeErasedParameter {
-struct Formatter<char[Size]> {
+    const void* value;
-    bool parse(StringView) { return true; }
+    bool (*formatter)(StringBuilder& builder, const void* value, StringView flags);
-    void format(StringBuilder& builder, const char* value) { builder.append(value); }
 };
 
 template<>
 struct Formatter<StringView> {
-    bool parse(StringView flags) { return flags.is_empty(); }
+    bool parse(StringView flags);
-    void format(StringBuilder& builder, StringView value) { builder.append(value); }
+    void format(StringBuilder& builder, StringView value);
+};
+template<size_t Size>
+struct Formatter<char[Size]> : Formatter<StringView> {
 };
 template<>
-struct Formatter<String> {
+struct Formatter<String> : Formatter<StringView> {
-    bool parse(StringView flags) { return flags.is_empty(); }
-    void format(StringBuilder& builder, const String& value) { builder.append(value); }
 };
 
 template<typename T>
@@ -94,19 +86,11 @@ struct Formatter<T, typename EnableIf<IsIntegral<T>::value>::Type> {
 };
 
 template<typename... Parameters>
-String format(StringView fmtstr, const Parameters&... parameters)
+Array<TypeErasedParameter, sizeof...(Parameters)> make_type_erased_parameters(const Parameters&... parameters)
 {
-    Array<Detail::Format::TypeErasedFormatter, sizeof...(parameters)> formatters { Detail::Format::make_type_erased_formatter(parameters)... };
+    return { TypeErasedParameter { &parameters, Detail::Format::format_value<Parameters> }... };
-    return Detail::Format::format(fmtstr, formatters);
-}
-template<typename... Parameters>
-void format(StringBuilder& builder, StringView fmtstr, const Parameters&... parameters)
-{
-    Array<Detail::Format::TypeErasedFormatter, sizeof...(parameters)> formatters { Detail::Format::make_type_erased_formatter(parameters)... };
-    Detail::Format::format(builder, fmtstr, formatters);
 }
 
-template<typename... Parameters>
+void vformat(StringBuilder& builder, StringView fmtstr, Span<const TypeErasedParameter>, size_t argument_index = 0);
-void StringBuilder::appendff(StringView fmtstr, const Parameters&... parameters) { AK::format(*this, fmtstr, parameters...); }
 
 } // namespace AK

AK/LogStream.cpp

@@ -27,6 +27,7 @@
 #include <AK/FlyString.h>
 #include <AK/LogStream.h>
 #include <AK/String.h>
+#include <AK/StringBuilder.h>
 #include <AK/StringView.h>
 
 #ifdef KERNEL

AK/String.cpp

@@ -216,7 +216,7 @@ Optional<unsigned> String::to_uint() const
 }
 
 template<typename T>
-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<const TypeErasedParameter> parameters)
+{
+    StringBuilder builder;
+    vformat(builder, fmtstr, parameters);
+    return builder.to_string();
+}
+
 }

AK/String.h

@@ -26,10 +26,10 @@
 
 #pragma once
 
+#include <AK/Format.h>
 #include <AK/Forward.h>
 #include <AK/RefPtr.h>
 #include <AK/Stream.h>
-#include <AK/StringBuilder.h>
 #include <AK/StringImpl.h>
 #include <AK/StringUtils.h>
 #include <AK/Traits.h>
@@ -239,6 +239,15 @@ public:
 
     static String format(const char*, ...);
 
+    static String vformatted(StringView fmtstr, Span<const TypeErasedParameter>);
+
+    template<typename... Parameters>
+    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<typename T>
     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)
+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;
-        }
-    }
-}
 
 }
 

AK/StringBuilder.h

@@ -27,6 +27,7 @@
 #pragma once
 
 #include <AK/ByteBuffer.h>
+#include <AK/Format.h>
 #include <AK/Forward.h>
 #include <AK/StringView.h>
 #include <stdarg.h>
@@ -48,9 +49,12 @@ public:
     void appendf(const char*, ...);
     void appendvf(const char*, va_list);
 
-    // Implemented in <AK/Format.h> to break circular dependency.
     template<typename... Parameters>
-    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;

AK/Tests/TestFormat.cpp

@@ -26,43 +26,44 @@
 
 #include <AK/TestSuite.h>
 
-#include <AK/Format.h>
+#include <AK/String.h>
+#include <AK/StringBuilder.h>
 
 TEST_CASE(format_string_literals)
 {
-    EXPECT_EQ(AK::format("prefix-{}-suffix", "abc"), "prefix-abc-suffix");
+    EXPECT_EQ(String::formatted("prefix-{}-suffix", "abc"), "prefix-abc-suffix");
-    EXPECT_EQ(AK::format("{}{}{}", "a", "b", "c"), "abc");
+    EXPECT_EQ(String::formatted("{}{}{}", "a", "b", "c"), "abc");
 }
 
 TEST_CASE(format_integers)
 {
-    EXPECT_EQ(AK::format("{}", 42u), "42");
+    EXPECT_EQ(String::formatted("{}", 42u), "42");
-    EXPECT_EQ(AK::format("{:4}", 42u), "  42");
+    EXPECT_EQ(String::formatted("{:4}", 42u), "  42");
-    EXPECT_EQ(AK::format("{:08}", 42u), "00000042");
+    EXPECT_EQ(String::formatted("{:08}", 42u), "00000042");
-    // EXPECT_EQ(AK::format("{:7}", -17), "    -17");
+    // EXPECT_EQ(String::formatted("{:7}", -17), "    -17");
-    EXPECT_EQ(AK::format("{}", -17), "-17");
+    EXPECT_EQ(String::formatted("{}", -17), "-17");
-    EXPECT_EQ(AK::format("{:04}", 13), "0013");
+    EXPECT_EQ(String::formatted("{:04}", 13), "0013");
-    EXPECT_EQ(AK::format("{:08x}", 4096), "00001000");
+    EXPECT_EQ(String::formatted("{:08x}", 4096), "00001000");
-    EXPECT_EQ(AK::format("{:x}", 0x1111222233334444ull), "1111222233334444");
+    EXPECT_EQ(String::formatted("{:x}", 0x1111222233334444ull), "1111222233334444");
 }
 
 TEST_CASE(reorder_format_arguments)
 {
-    EXPECT_EQ(AK::format("{1}{0}", "a", "b"), "ba");
+    EXPECT_EQ(String::formatted("{1}{0}", "a", "b"), "ba");
-    EXPECT_EQ(AK::format("{0}{1}", "a", "b"), "ab");
+    EXPECT_EQ(String::formatted("{0}{1}", "a", "b"), "ab");
-    EXPECT_EQ(AK::format("{0}{0}{0}", "a", "b"), "aaa");
+    EXPECT_EQ(String::formatted("{0}{0}{0}", "a", "b"), "aaa");
-    EXPECT_EQ(AK::format("{1}{}{0}", "a", "b", "c"), "baa");
+    EXPECT_EQ(String::formatted("{1}{}{0}", "a", "b", "c"), "baa");
 }
 
 TEST_CASE(escape_braces)
 {
-    EXPECT_EQ(AK::format("{{{}", "foo"), "{foo");
+    EXPECT_EQ(String::formatted("{{{}", "foo"), "{foo");
-    EXPECT_EQ(AK::format("{}}}", "bar"), "bar}");
+    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)

Libraries/LibGUI/Clipboard.h

@@ -26,6 +26,7 @@
 
 #pragma once
 
+#include <AK/ByteBuffer.h>
 #include <AK/Function.h>
 #include <AK/HashMap.h>
 #include <AK/String.h>