[NFC][Support] Refactor FormatVariadic code. (#106610)

jurahul · web-flow · commit c2018fa40fd0 · 2024-09-05T09:01:19.000-07:00
- Rename `Align` field in ReplacementItem/FmtAlign to `Width` to 
  accurately reflect its use.
- Change both `Width` and `Index` in ReplacementItem to 32-bit int 
  instead of size_t (as 64-bits seems excessive in this context).
- Eliminate the use of `Empty` ReplacementType, and use the
  existing std::optional&lt;&gt; instead to indicate that.
- Eliminate some boilerplate type code in formatv().
- Eliminate the loop in `splitLiteralAndReplacement`. The existing
  code will never loop back.
- Directly use constructor instead of std::make_pair.
diff --git a/llvm/include/llvm/Support/FormatCommon.h b/llvm/include/llvm/Support/FormatCommon.h
@@ -16,43 +16,45 @@
 namespace llvm {
 enum class AlignStyle { Left, Center, Right };
 
+/// Helper class to format to a \p Width wide field, with alignment \p Where
+/// within that field.
 struct FmtAlign {
   support::detail::format_adapter &Adapter;
   AlignStyle Where;
-  size_t Amount;
+  unsigned Width;
   char Fill;
 
   FmtAlign(support::detail::format_adapter &Adapter, AlignStyle Where,
-           size_t Amount, char Fill = ' ')
-      : Adapter(Adapter), Where(Where), Amount(Amount), Fill(Fill) {}
+           unsigned Width, char Fill = ' ')
+      : Adapter(Adapter), Where(Where), Width(Width), Fill(Fill) {}
 
   void format(raw_ostream &S, StringRef Options) {
     // If we don't need to align, we can format straight into the underlying
     // stream.  Otherwise we have to go through an intermediate stream first
     // in order to calculate how long the output is so we can align it.
     // TODO: Make the format method return the number of bytes written, that
     // way we can also skip the intermediate stream for left-aligned output.
-    if (Amount == 0) {
+    if (Width == 0) {
       Adapter.format(S, Options);
       return;
     }
     SmallString<64> Item;
     raw_svector_ostream Stream(Item);
 
     Adapter.format(Stream, Options);
-    if (Amount <= Item.size()) {
+    if (Width <= Item.size()) {
       S << Item;
       return;
     }
 
-    size_t PadAmount = Amount - Item.size();
+    unsigned PadAmount = Width - static_cast<unsigned>(Item.size());
     switch (Where) {
     case AlignStyle::Left:
       S << Item;
       fill(S, PadAmount);
       break;
     case AlignStyle::Center: {
-      size_t X = PadAmount / 2;
+      unsigned X = PadAmount / 2;
       fill(S, X);
       S << Item;
       fill(S, PadAmount - X);
@@ -66,8 +68,8 @@ struct FmtAlign {
   }
 
 private:
-  void fill(llvm::raw_ostream &S, size_t Count) {
-    for (size_t I = 0; I < Count; ++I)
+  void fill(llvm::raw_ostream &S, unsigned Count) {
+    for (unsigned I = 0; I < Count; ++I)
       S << Fill;
   }
 };
diff --git a/llvm/include/llvm/Support/FormatVariadic.h b/llvm/include/llvm/Support/FormatVariadic.h
@@ -43,21 +43,20 @@
 
 namespace llvm {
 
-enum class ReplacementType { Empty, Format, Literal };
+enum class ReplacementType { Format, Literal };
 
 struct ReplacementItem {
-  ReplacementItem() = default;
   explicit ReplacementItem(StringRef Literal)
       : Type(ReplacementType::Literal), Spec(Literal) {}
-  ReplacementItem(StringRef Spec, size_t Index, size_t Align, AlignStyle Where,
-                  char Pad, StringRef Options)
-      : Type(ReplacementType::Format), Spec(Spec), Index(Index), Align(Align),
+  ReplacementItem(StringRef Spec, unsigned Index, unsigned Width,
+                  AlignStyle Where, char Pad, StringRef Options)
+      : Type(ReplacementType::Format), Spec(Spec), Index(Index), Width(Width),
         Where(Where), Pad(Pad), Options(Options) {}
 
-  ReplacementType Type = ReplacementType::Empty;
+  ReplacementType Type;
   StringRef Spec;
-  size_t Index = 0;
-  size_t Align = 0;
+  unsigned Index = 0;
+  unsigned Width = 0;
   AlignStyle Where = AlignStyle::Right;
   char Pad = 0;
   StringRef Options;
@@ -81,8 +80,6 @@ class formatv_object_base {
   void format(raw_ostream &S) const {
     const auto Replacements = parseFormatString(Fmt, Adapters.size(), Validate);
     for (const auto &R : Replacements) {
-      if (R.Type == ReplacementType::Empty)
-        continue;
       if (R.Type == ReplacementType::Literal) {
         S << R.Spec;
         continue;
@@ -94,7 +91,7 @@ class formatv_object_base {
 
       auto *W = Adapters[R.Index];
 
-      FmtAlign Align(*W, R.Where, R.Align, R.Pad);
+      FmtAlign Align(*W, R.Where, R.Width, R.Pad);
       Align.format(S, R.Options);
     }
   }
@@ -248,14 +245,10 @@ template <typename Tuple> class formatv_object : public formatv_object_base {
 
 // formatv() with validation enable/disable controlled by the first argument.
 template <typename... Ts>
-inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals)
-    -> formatv_object<decltype(std::make_tuple(
-        support::detail::build_format_adapter(std::forward<Ts>(Vals))...))> {
-  using ParamTuple = decltype(std::make_tuple(
-      support::detail::build_format_adapter(std::forward<Ts>(Vals))...));
+inline auto formatv(bool Validate, const char *Fmt, Ts &&...Vals) {
   auto Params = std::make_tuple(
       support::detail::build_format_adapter(std::forward<Ts>(Vals))...);
-  return formatv_object<ParamTuple>(Fmt, std::move(Params), Validate);
+  return formatv_object<decltype(Params)>(Fmt, std::move(Params), Validate);
 }
 
 // formatv() with validation enabled.
diff --git a/llvm/lib/Support/FormatVariadic.cpp b/llvm/lib/Support/FormatVariadic.cpp
@@ -26,7 +26,7 @@ static std::optional<AlignStyle> translateLocChar(char C) {
 }
 
 static bool consumeFieldLayout(StringRef &Spec, AlignStyle &Where,
-                               size_t &Align, char &Pad) {
+                               unsigned &Align, char &Pad) {
   Where = AlignStyle::Right;
   Align = 0;
   Pad = ' ';
@@ -60,14 +60,14 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
   // If the replacement sequence does not start with a non-negative integer,
   // this is an error.
   char Pad = ' ';
-  std::size_t Align = 0;
+  unsigned Align = 0;
   AlignStyle Where = AlignStyle::Right;
   StringRef Options;
-  size_t Index = 0;
+  unsigned Index = 0;
   RepString = RepString.trim();
   if (RepString.consumeInteger(0, Index)) {
     assert(false && "Invalid replacement sequence index!");
-    return ReplacementItem{};
+    return std::nullopt;
   }
   RepString = RepString.trim();
   if (RepString.consume_front(",")) {
@@ -83,61 +83,50 @@ static std::optional<ReplacementItem> parseReplacementItem(StringRef Spec) {
   assert(RepString.empty() &&
          "Unexpected characters found in replacement string!");
 
-  return ReplacementItem{Spec, Index, Align, Where, Pad, Options};
+  return ReplacementItem(Spec, Index, Align, Where, Pad, Options);
 }
 
-static std::pair<ReplacementItem, StringRef>
+static std::pair<std::optional<ReplacementItem>, StringRef>
 splitLiteralAndReplacement(StringRef Fmt) {
-  while (!Fmt.empty()) {
-    // Everything up until the first brace is a literal.
-    if (Fmt.front() != '{') {
-      std::size_t BO = Fmt.find_first_of('{');
-      return std::make_pair(ReplacementItem{Fmt.substr(0, BO)}, Fmt.substr(BO));
-    }
-
-    StringRef Braces = Fmt.take_while([](char C) { return C == '{'; });
-    // If there is more than one brace, then some of them are escaped.  Treat
-    // these as replacements.
-    if (Braces.size() > 1) {
-      size_t NumEscapedBraces = Braces.size() / 2;
-      StringRef Middle = Fmt.take_front(NumEscapedBraces);
-      StringRef Right = Fmt.drop_front(NumEscapedBraces * 2);
-      return std::make_pair(ReplacementItem{Middle}, Right);
-    }
-    // An unterminated open brace is undefined. Assert to indicate that this is
-    // undefined and that we consider it an error. When asserts are disabled,
-    // build a replacement item with an error message.
-    std::size_t BC = Fmt.find_first_of('}');
-    if (BC == StringRef::npos) {
-      assert(
-          false &&
-          "Unterminated brace sequence. Escape with {{ for a literal brace.");
-      return std::make_pair(
-          ReplacementItem{"Unterminated brace sequence. Escape with {{ for a "
-                          "literal brace."},
-          StringRef());
-    }
+  assert(!Fmt.empty());
+  // Everything up until the first brace is a literal.
+  if (Fmt.front() != '{') {
+    size_t BO = Fmt.find_first_of('{');
+    return {ReplacementItem{Fmt.substr(0, BO)}, Fmt.substr(BO)};
+  }
 
-    // Even if there is a closing brace, if there is another open brace before
-    // this closing brace, treat this portion as literal, and try again with the
-    // next one.
-    std::size_t BO2 = Fmt.find_first_of('{', 1);
-    if (BO2 < BC)
-      return std::make_pair(ReplacementItem{Fmt.substr(0, BO2)},
-                            Fmt.substr(BO2));
+  StringRef Braces = Fmt.take_while([](char C) { return C == '{'; });
+  // If there is more than one brace, then some of them are escaped.  Treat
+  // these as replacements.
+  if (Braces.size() > 1) {
+    size_t NumEscapedBraces = Braces.size() / 2;
+    StringRef Middle = Fmt.take_front(NumEscapedBraces);
+    StringRef Right = Fmt.drop_front(NumEscapedBraces * 2);
+    return {ReplacementItem(Middle), Right};
+  }
+  // An unterminated open brace is undefined. Assert to indicate that this is
+  // undefined and that we consider it an error. When asserts are disabled,
+  // build a replacement item with an error message.
+  size_t BC = Fmt.find_first_of('}');
+  if (BC == StringRef::npos) {
+    assert(false &&
+           "Unterminated brace sequence. Escape with {{ for a literal brace.");
+    return {ReplacementItem("Unterminated brace sequence. Escape with {{ for a "
+                            "literal brace."),
+            StringRef()};
+  }
 
-    StringRef Spec = Fmt.slice(1, BC);
-    StringRef Right = Fmt.substr(BC + 1);
+  // Even if there is a closing brace, if there is another open brace before
+  // this closing brace, treat this portion as literal, and try again with the
+  // next one.
+  size_t BO2 = Fmt.find_first_of('{', 1);
+  if (BO2 < BC)
+    return {ReplacementItem(Fmt.substr(0, BO2)), Fmt.substr(BO2)};
 
-    auto RI = parseReplacementItem(Spec);
-    if (RI)
-      return std::make_pair(*RI, Right);
+  StringRef Spec = Fmt.slice(1, BC);
+  StringRef Right = Fmt.substr(BC + 1);
 
-    // If there was an error parsing the replacement item, treat it as an
-    // invalid replacement spec, and just continue.
-    Fmt = Fmt.drop_front(BC + 1);
-  }
-  return std::make_pair(ReplacementItem{Fmt}, StringRef());
+  return {parseReplacementItem(Spec), Right};
 }
 
 #ifndef NDEBUG
@@ -153,17 +142,18 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
 
 #if ENABLE_VALIDATION
   const StringRef SavedFmtStr = Fmt;
-  size_t NumExpectedArgs = 0;
+  unsigned NumExpectedArgs = 0;
 #endif
 
   while (!Fmt.empty()) {
-    ReplacementItem I;
+    std::optional<ReplacementItem> I;
     std::tie(I, Fmt) = splitLiteralAndReplacement(Fmt);
-    if (I.Type != ReplacementType::Empty)
-      Replacements.push_back(I);
+    if (!I)
+      continue;
+    Replacements.emplace_back(*I);
 #if ENABLE_VALIDATION
-    if (I.Type == ReplacementType::Format)
-      NumExpectedArgs = std::max(NumExpectedArgs, I.Index + 1);
+    if (I->Type == ReplacementType::Format)
+      NumExpectedArgs = std::max(NumExpectedArgs, I->Index + 1);
 #endif
   }
 
@@ -195,7 +185,7 @@ formatv_object_base::parseFormatString(StringRef Fmt, size_t NumArgs,
   // Find the number of unique indices seen. All replacement indices
   // are < NumExpectedArgs.
   SmallVector<bool> Indices(NumExpectedArgs);
-  size_t Count = 0;
+  unsigned Count = 0;
   for (const ReplacementItem &I : Replacements) {
     if (I.Type != ReplacementType::Format || Indices[I.Index])
       continue;
diff --git a/llvm/unittests/Support/FormatVariadicTest.cpp b/llvm/unittests/Support/FormatVariadicTest.cpp
