Browse Source

YT-21868: Refactor NYT::Format

NYT::Format had several problems:
1. There are too many ways to enable printing of T. Not all are equally good. You could specialize TValueFormatter, you could write an overload of FormatValue, you could write an overload of ToString, you could write an overload of operator << for special stream or you could specialize the Out function.
2. If you attempt to print T which cannot be printed, you get a linker error without a proper source location which is very frustrating to work with.
3. There is no static analysis of format string performed even when it is possible.
4. If you write FormatValue overload, you still have to write ToString overload if you want to use this function (and people tend to use it quite a bit, since it is defined for util types and enums.

This pr addresses these issues to some extent. Relevant changes:
1. The only way to support NYT::Format is to define the FormatValue overload. Otherwise, you get a compile-time error.
2. Format overloads have changed: Now you have two options for general use:
```
TString Format(TStaticFormat<TArgs...> fmt, TArgs&&... args);
TString Format(TRuntimeFormat fmt, TArgs&&... args);
```
Either overload checks if TArg has a FormatValue overload. TStaticFormat performs a compile-time check of flags and the argument count. It binds to any string literal and constexpr string/string_view (and TStringBuf). TRuntimeFormat has to be mentioned explicitly. Otherwise, you will get a compile-time error for using runtime variable as a format.

3(!!!). Types which name begins with NYT:: have a specialization of ToString function which uses FormatValue. Thus, if you write class in namespace NYT and define FormatValue, you get ToString automatically. If your type is not from namespace enclosing NYT, you can just call NYT::ToString for the same effect. This limitation was caused by the fact, that we cannot review all of the external projects code which might inherit from stl  classes or adopt some other questionable code practises which may completely break the dispatching mechanism of ToString due to the specialization (there were such cases). Proper documentation of this library will be added soon, so that this interaction is made known. This limitation might be lifted later
77beb68082e10aaf48be1842aad8aba63f26c1bd
arkady-e1ppa 9 months ago
parent
commit
327232940e

+ 1 - 1
library/cpp/yt/logging/logger-inl.h

@@ -24,7 +24,7 @@ inline bool TLogger::IsAnchorUpToDate(const TLoggingAnchor& position) const
 template <class... TArgs>
 void TLogger::AddTag(const char* format, TArgs&&... args)
 {
-    AddRawTag(Format(format, std::forward<TArgs>(args)...));
+    AddRawTag(Format(TRuntimeFormat{format}, std::forward<TArgs>(args)...));
 }
 
 template <class TType>

+ 1 - 1
library/cpp/yt/logging/logger.h

@@ -22,7 +22,7 @@
 
 #include <atomic>
 
-#if (__clang__ || __clang_major__ < 16)
+#if (!__clang__ || __clang_major__ < 16)
     #define YT_DISABLE_FORMAT_STATIC_ANALYSIS
 #endif
 

+ 10 - 3
library/cpp/yt/logging/static_analysis-inl.h

@@ -6,7 +6,7 @@
 
 #include <library/cpp/yt/misc/preprocessor.h>
 
-#include <library/cpp/yt/string/format_analyser.h>
+#include <library/cpp/yt/string/format.h>
 
 #include <string_view>
 #include <variant> // monostate
@@ -20,6 +20,12 @@ template <class T>
 struct TLoggerFormatArg
 { };
 
+// Required for TLoggerFormatArg to inherit CFormattable concept
+// from T.
+template <class T>
+    requires CFormattable<T>
+void FormatValue(TStringBuilderBase*, const TLoggerFormatArg<T>&, TStringBuf);
+
 ////////////////////////////////////////////////////////////////////////////////
 
 // Stateless constexpr way of capturing arg types
@@ -32,7 +38,8 @@ struct TLoggerFormatArgs
 
 // Used for macro conversion. Purposefully undefined.
 template <class... TArgs>
-TLoggerFormatArgs<TArgs...> AsFormatArgs(TArgs&&...);
+TLoggerFormatArgs<std::remove_cvref_t<TArgs>...>
+AsFormatArgs(TArgs&&...);
 
 ////////////////////////////////////////////////////////////////////////////////
 
@@ -135,5 +142,5 @@ struct NYT::TFormatArg<NYT::NLogging::NDetail::TLoggerFormatArg<T>>
     // "\"Value: %\" \"u\""
     // Thus adding a \" \" sequence.
     static constexpr auto FlagSpecifiers
-        = TFormatArgBase::ExtendFlags</*Hot*/ false, 2, std::array{'\"', ' '}, /*TFrom*/ NYT::TFormatArg<T>>();
+        = TFormatArgBase::ExtendFlags</*Hot*/ false, 2, std::array{'\"', ' '}, /*TFrom*/ T>();
 };

+ 10 - 8
library/cpp/yt/memory/ref.cpp

@@ -6,6 +6,8 @@
 
 #include <library/cpp/yt/misc/port.h>
 
+#include <library/cpp/yt/string/format.h>
+
 #include <util/system/info.h>
 #include <util/system/align.h>
 
@@ -304,24 +306,24 @@ TSharedMutableRef TSharedMutableRef::MakeCopy(TRef ref, TRefCountedTypeCookie ta
 
 ////////////////////////////////////////////////////////////////////////////////
 
-TString ToString(TRef ref)
+void FormatValue(TStringBuilderBase* builder, const TRef& ref, TStringBuf spec)
 {
-    return TString(ref.Begin(), ref.End());
+    FormatValue(builder, TStringBuf{ref.Begin(), ref.End()}, spec);
 }
 
-TString ToString(const TMutableRef& ref)
+void FormatValue(TStringBuilderBase* builder, const TMutableRef& ref, TStringBuf spec)
 {
-    return ToString(TRef(ref));
+    FormatValue(builder, TRef(ref), spec);
 }
 
-TString ToString(const TSharedRef& ref)
+void FormatValue(TStringBuilderBase* builder, const TSharedRef& ref, TStringBuf spec)
 {
-    return ToString(TRef(ref));
+    FormatValue(builder, TRef(ref), spec);
 }
 
-TString ToString(const TSharedMutableRef& ref)
+void FormatValue(TStringBuilderBase* builder, const TSharedMutableRef& ref, TStringBuf spec)
 {
-    return ToString(TRef(ref));
+    FormatValue(builder, TRef(ref), spec);
 }
 
 size_t GetPageSize()

+ 6 - 4
library/cpp/yt/memory/ref.h

@@ -4,6 +4,8 @@
 #include "range.h"
 #include "shared_range.h"
 
+#include <library/cpp/yt/string/format.h>
+
 #include <type_traits>
 
 namespace NYT {
@@ -381,10 +383,10 @@ private:
 
 ////////////////////////////////////////////////////////////////////////////////
 
-TString ToString(TRef ref);
-TString ToString(const TMutableRef& ref);
-TString ToString(const TSharedRef& ref);
-TString ToString(const TSharedMutableRef& ref);
+void FormatValue(TStringBuilderBase* builder, const TRef& ref, TStringBuf spec);
+void FormatValue(TStringBuilderBase* builder, const TMutableRef& ref, TStringBuf spec);
+void FormatValue(TStringBuilderBase* builder, const TSharedRef& ref, TStringBuf spec);
+void FormatValue(TStringBuilderBase* builder, const TSharedMutableRef& ref, TStringBuf);
 
 size_t GetPageSize();
 size_t RoundUpToPage(size_t bytes);

+ 2 - 0
library/cpp/yt/misc/wrapper_traits.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include <util/generic/strbuf.h>
+
 #include <concepts>
 #include <utility>
 

File diff suppressed because it is too large
+ 559 - 398
library/cpp/yt/string/format-inl.h


+ 61 - 11
library/cpp/yt/string/format.h

@@ -54,15 +54,15 @@ namespace NYT {
  *
  */
 
-template <size_t Length, class... TArgs>
-void Format(TStringBuilderBase* builder, const char (&format)[Length], TArgs&&... args);
 template <class... TArgs>
-void Format(TStringBuilderBase* builder, TStringBuf format, TArgs&&... args);
+void Format(TStringBuilderBase* builder, TStaticFormat<TArgs...> fmt, TArgs&&... args);
+template <class... TArgs>
+void Format(TStringBuilderBase* builder, TRuntimeFormat fmt, TArgs&&... args);
 
-template <size_t Length, class... TArgs>
-TString Format(const char (&format)[Length], TArgs&&... args);
 template <class... TArgs>
-TString Format(TStringBuf format, TArgs&&... args);
+TString Format(TStaticFormat<TArgs...> fmt, TArgs&&... args);
+template <class... TArgs>
+TString Format(TRuntimeFormat fmt, TArgs&&... args);
 
 ////////////////////////////////////////////////////////////////////////////////
 
@@ -101,6 +101,19 @@ struct TFormatterWrapper
     TFormatter Formatter;
 };
 
+// Allows insertion of text conditionally.
+// Usage:
+/*
+ NYT::Format(
+    "Value is %v%v",
+    42,
+    MakeFormatterWrapper([&] (auto* builder) {
+        If (PossiblyMissingInfo_) {
+            builder->AppendString(", PossiblyMissingInfo: ");
+            FormatValue(builder, PossiblyMissingInfo_, "v");
+        }
+    }));
+ */
 template <class TFormatter>
 TFormatterWrapper<TFormatter> MakeFormatterWrapper(
     TFormatter&& formatter);
@@ -114,7 +127,7 @@ template <class... TArgs>
 void FormatValue(
     TStringBuilderBase* builder,
     const TLazyMultiValueFormatter<TArgs...>& value,
-    TStringBuf /*format*/);
+    TStringBuf /*spec*/);
 
 //! A wrapper for a bunch of values that formats them lazily on demand.
 /*!
@@ -129,12 +142,19 @@ class TLazyMultiValueFormatter
     : private TNonCopyable
 {
 public:
-    TLazyMultiValueFormatter(TStringBuf format, TArgs&&... args);
-
+    TLazyMultiValueFormatter(TStringBuf fmt, TArgs&&... args);
+
+    // NB(arkady-e1ppa): We actually have to
+    // forward declare this method as above
+    // and friend-declare it as specialization
+    // here because clang is stupid and would
+    // treat this friend declartion as a hidden friend
+    // declaration which in turn is treated as a separate symbol
+    // causing linker to not find the actual definition.
     friend void FormatValue<>(
         TStringBuilderBase* builder,
         const TLazyMultiValueFormatter& value,
-        TStringBuf /*format*/);
+        TStringBuf /*spec*/);
 
 private:
     const TStringBuf Format_;
@@ -142,7 +162,37 @@ private:
 };
 
 template <class ... Args>
-auto MakeLazyMultiValueFormatter(TStringBuf format, Args&&... args);
+auto MakeLazyMultiValueFormatter(TStringBuf fmt, Args&&... args);
+
+////////////////////////////////////////////////////////////////////////////////
+
+/*
+    Example:
+
+    FormatVector("One: %v, Two: %v, Three: %v", {1, 2, 3})
+    => "One: 1, Two: 2, Three: 3"
+*/
+template <size_t Length, class TVector>
+void FormatVector(
+    TStringBuilderBase* builder,
+    const char (&fmt)[Length],
+    const TVector& vec);
+
+template <class TVector>
+void FormatVector(
+    TStringBuilderBase* builder,
+    TStringBuf fmt,
+    const TVector& vec);
+
+template <size_t Length, class TVector>
+TString FormatVector(
+    const char (&fmt)[Length],
+    const TVector& vec);
+
+template <class TVector>
+TString FormatVector(
+    TStringBuf fmt,
+    const TVector& vec);
 
 ////////////////////////////////////////////////////////////////////////////////
 

+ 6 - 37
library/cpp/yt/string/format_analyser-inl.h

@@ -4,11 +4,11 @@
 #include "format_analyser.h"
 #endif
 
-namespace NYT {
+#include "format_arg.h"
 
-////////////////////////////////////////////////////////////////////////////////
+namespace NYT::NDetail {
 
-namespace NDetail {
+////////////////////////////////////////////////////////////////////////////////
 
 consteval bool Contains(std::string_view sv, char symbol)
 {
@@ -93,7 +93,9 @@ consteval void TFormatAnalyser::ValidateFormat(std::string_view fmt)
 template <class TArg>
 consteval auto TFormatAnalyser::GetSpecifiers()
 {
-    static_assert(CFormattable<TArg>, "Your specialization of TFormatArg is broken");
+    if constexpr (!CFormattable<TArg>) {
+        CrashCompilerNotFormattable<TArg>("Your specialization of TFormatArg is broken");
+    }
 
     return TSpecifiers{
         .Conversion = std::string_view{
@@ -105,39 +107,6 @@ consteval auto TFormatAnalyser::GetSpecifiers()
     };
 }
 
-} // namespace NDetail
-
 ////////////////////////////////////////////////////////////////////////////////
 
-template <bool Hot, size_t N, std::array<char, N> List, class TFrom>
-consteval auto TFormatArgBase::ExtendConversion()
-{
-    static_assert(CFormattable<TFrom>);
-    return AppendArrays<Hot, N, List, &TFrom::ConversionSpecifiers>();
-}
-
-template <bool Hot, size_t N, std::array<char, N> List, class TFrom>
-consteval auto TFormatArgBase::ExtendFlags()
-{
-    static_assert(CFormattable<TFrom>);
-    return AppendArrays<Hot, N, List, &TFrom::FlagSpecifiers>();
-}
-
-template <bool Hot, size_t N, std::array<char, N> List, auto* From>
-consteval auto TFormatArgBase::AppendArrays()
-{
-    auto& from = *From;
-    return [] <size_t... I, size_t... J> (
-        std::index_sequence<I...>,
-        std::index_sequence<J...>) {
-            if constexpr (Hot) {
-                return std::array{List[J]..., from[I]...};
-            } else {
-                return std::array{from[I]..., List[J]...};
-            }
-        } (
-            std::make_index_sequence<std::size(from)>(),
-            std::make_index_sequence<N>());
-    }
-
 } // namespace NYT::NDetail

+ 7 - 62
library/cpp/yt/string/format_analyser.h

@@ -1,14 +1,14 @@
 #pragma once
 
+#include <util/generic/strbuf.h>
+
 #include <array>
 #include <string_view>
 
-namespace NYT {
+namespace NYT::NDetail {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-namespace NDetail {
-
 struct TFormatAnalyser
 {
 public:
@@ -18,6 +18,9 @@ public:
 private:
     // Non-constexpr function call will terminate compilation.
     // Purposefully undefined and non-constexpr/consteval
+    template <class T>
+    static void CrashCompilerNotFormattable(std::string_view /*msg*/)
+    { /*Suppress "internal linkage but undefined" warning*/ }
     static void CrashCompilerNotEnoughArguments(std::string_view msg);
     static void CrashCompilerTooManyArguments(std::string_view msg);
     static void CrashCompilerWrongTermination(std::string_view msg);
@@ -35,67 +38,9 @@ private:
     static constexpr char IntroductorySymbol = '%';
 };
 
-} // namespace NDetail
-
 ////////////////////////////////////////////////////////////////////////////////
 
-// Base used for flag checks for each type independently.
-// Use it for overrides.
-struct TFormatArgBase
-{
-public:
-    // TODO(arkady-e1ppa): Consider more strict formatting rules.
-    static constexpr std::array ConversionSpecifiers = {
-            'v', '1', 'c', 's', 'd', 'i', 'o',
-            'x', 'X', 'u', 'f', 'F', 'e', 'E',
-            'a', 'A', 'g', 'G', 'n', 'p'
-        };
-
-    static constexpr std::array FlagSpecifiers = {
-        '-', '+', ' ', '#', '0',
-        '1', '2', '3', '4', '5',
-        '6', '7', '8', '9',
-        '*', '.', 'h', 'l', 'j',
-        'z', 't', 'L', 'q', 'Q'
-    };
-
-    template <class T>
-    static constexpr bool IsSpecifierList = requires (T t) {
-        [] <size_t N> (std::array<char, N>) { } (t);
-    };
-
-    // Hot = |true| adds specifiers to the beggining
-    // of a new array.
-    template <bool Hot, size_t N, std::array<char, N> List, class TFrom = TFormatArgBase>
-    static consteval auto ExtendConversion();
-
-    template <bool Hot, size_t N, std::array<char, N> List, class TFrom = TFormatArgBase>
-    static consteval auto ExtendFlags();
-
-private:
-    template <bool Hot, size_t N, std::array<char, N> List, auto* From>
-    static consteval auto AppendArrays();
-};
-
-////////////////////////////////////////////////////////////////////////////////
-
-template <class T>
-struct TFormatArg
-    : public TFormatArgBase
-{ };
-
-// Semantic requirement:
-// Said field must be constexpr.
-template <class T>
-concept CFormattable = requires {
-    TFormatArg<T>::ConversionSpecifiers;
-    requires TFormatArgBase::IsSpecifierList<decltype(TFormatArg<T>::ConversionSpecifiers)>;
-
-    TFormatArg<T>::FlagSpecifiers;
-    requires TFormatArgBase::IsSpecifierList<decltype(TFormatArg<T>::FlagSpecifiers)>;
-};
-
-} // namespace NYT
+} // namespace NYT::NDetail
 
 #define FORMAT_ANALYSER_INL_H_
 #include "format_analyser-inl.h"

Some files were not shown because too many files changed in this diff