[StringExtras] Add a helper class for comma-separated lists

[kazu]

Summary

This patch introduces a helper class SubsequentDelim to simplify loops
that generate a comma-separated lists.

For example, consider the following loop, taken from
llvm/lib/CodeGen/MachineBasicBlock.cpp:

for (auto I = pred_begin(), E = pred_end(); I != E; ++I) {
  if (I != pred_begin())
    OS << ", ";
  OS << printMBBReference(**I);
}

The new class allows us to rewrite the loop as:

SubsequentDelim SD;
for (auto I = pred_begin(), E = pred_end(); I != E; ++I)
  OS << SD << printMBBReference(**I);

where SD evaluates to the empty string for the first time and ", " for
subsequent iterations.

Unlike interleaveComma, defined in llvm/include/llvm/ADT/STLExtras.h,
SubsequentDelim can accommodate a wider variety of loops, including:

• those that conditionally skip certain items,
• those that need iterators to call getSuccProbability(I), and
• those that iterate over integer ranges.

As an example, this patch cleans up MachineBasicBlock::print.

[dblaikie]

Sure, sounds alright.

Can do similar things with interleaveComma + various filtering and adapting iterators, but for many cases that's probably overly complicated.

[MaskRay]

The name SubsequentDelim looks a bit strange to me. Maybe others have suggestions on the name?
(personally I might choose AddDelimUnlessFirst)

[dblaikie]

In D94377#2491956, @MaskRay wrote:

The name SubsequentDelim looks a bit strange to me. Maybe others have suggestions on the name?
(personally I might choose AddDelimUnlessFirst)

Yeah, I don't have any particularly compelling names - maybe "StreamableSeparator"

[kazu]

In D94377#2493048, @dblaikie wrote:

In D94377#2491956, @MaskRay wrote:

The name SubsequentDelim looks a bit strange to me. Maybe others have suggestions on the name?
(personally I might choose AddDelimUnlessFirst)

Yeah, I don't have any particularly compelling names - maybe "StreamableSeparator"

How about ListSeparator? Windows seems to have regional setting called "List Separator" specifying what separator to use in a list, so I am not inventing a new term at least:

https://support.talkdesk.com/hc/en-us/articles/360047099811-CSV-Files-and-Regional-Settings

I'd like to keep the name reasonably short, so I might sweep under the carpet the fact that the first invocation generates the empty string.

llvm/include/llvm/ADT/StringExtras.h

inline std::string join_items(Sep Separator, Args &&... Items) {
 
   size_t NS = detail::join_one_item_size(Separator);
   size_t NI = detail::join_items_size(std::forward<Args>(Items)...);
   Result.reserve(NI + (sizeof...(Items) - 1) * NS + 1);
   detail::join_items_impl(Result, Separator, std::forward<Args>(Items)...);
   return Result;
 }
 
+/// A helper class to return the specified delimiter string after the first
+/// invocation of operator StringRef().  Used to generate a comma-separated
+/// list from a loop like so:
+///
+/// \code
+///   SubsequentDelim SD;
+///   for (auto &I : C)
+///     OS << SD << I.getName();
+/// \end
+class SubsequentDelim {
+  bool First = true;
+  StringRef Delim;
+
+ public:
+  SubsequentDelim(StringRef Delim = ", ") : Delim(Delim) {}
+  operator StringRef() {
+    if (First) {
+      First = false;
+      return {};
+    }
+    return Delim;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_STRINGEXTRAS_H

llvm/lib/CodeGen/MachineBasicBlock.cpp

void MachineBasicBlock::print(raw_ostream &OS, ModuleSlotTracker &MST,
   const TargetInstrInfo &TII = *getParent()->getSubtarget().getInstrInfo();
   bool HasLineAttributes = false;
 
   // Print the preds of this block according to the CFG.
   if (!pred_empty() && IsStandalone) {
     if (Indexes) OS << '\t';
     // Don't indent(2), align with previous line attributes.
     OS << "; predecessors: ";
-    for (auto I = pred_begin(), E = pred_end(); I != E; ++I) {
-      if (I != pred_begin())
-        OS << ", ";
-      OS << printMBBReference(**I);
-    }
+    SubsequentDelim SD;
+    for (auto *Pred : predecessors())
+      OS << SD << printMBBReference(*Pred);
     OS << '\n';
     HasLineAttributes = true;
   }
 
   if (!succ_empty()) {
     if (Indexes) OS << '\t';
     // Print the successors
     OS.indent(2) << "successors: ";
+    SubsequentDelim SD;
     for (auto I = succ_begin(), E = succ_end(); I != E; ++I) {
-      if (I != succ_begin())
-        OS << ", ";
-      OS << printMBBReference(**I);
+      OS << SD << printMBBReference(**I);
       if (!Probs.empty())
         OS << '('
            << format("0x%08" PRIx32, getSuccProbability(I).getNumerator())
            << ')';
     }
     if (!Probs.empty() && IsStandalone) {
       // Print human readable probabilities as comments.
       OS << "; ";
+      SubsequentDelim SD;
       for (auto I = succ_begin(), E = succ_end(); I != E; ++I) {
         const BranchProbability &BP = getSuccProbability(I);
-        if (I != succ_begin())
-          OS << ", ";
-        OS << printMBBReference(**I) << '('
+        OS << SD << printMBBReference(**I) << '('
            << format("%.2f%%",
                      rint(((double)BP.getNumerator() / BP.getDenominator()) *
                           100.0 * 100.0) /
                          100.0)
            << ')';
       }
     }
 
     OS << '\n';
     HasLineAttributes = true;
   }
 
   if (!livein_empty() && MRI.tracksLiveness()) {
     if (Indexes) OS << '\t';
     OS.indent(2) << "liveins: ";
 
-    bool First = true;
+    SubsequentDelim SD;
     for (const auto &LI : liveins()) {
-      if (!First)
-        OS << ", ";
-      First = false;
-      OS << printReg(LI.PhysReg, TRI);
+      OS << SD << printReg(LI.PhysReg, TRI);
       if (!LI.LaneMask.all())
         OS << ":0x" << PrintLaneMask(LI.LaneMask);
     }
     HasLineAttributes = true;
   }
 
   if (HasLineAttributes)
     OS << '\n';

llvm/unittests/ADT/StringExtrasTest.cpp

 
 TEST(StringExtras, IToStr) {
   EXPECT_EQ("0", itostr(0));
   EXPECT_EQ("1", itostr(1));
   EXPECT_EQ("-1", itostr(-1));
   EXPECT_EQ(std::to_string(MinInt64), itostr(MinInt64));
   EXPECT_EQ(std::to_string(MaxInt64), itostr(MaxInt64));
 }
+
+TEST(StringExtras, SubsequentDelim) {
+  SubsequentDelim SD;
+  StringRef S = SD;
+  EXPECT_EQ(S, "");
+  S = SD;
+  EXPECT_EQ(S, ", ");
+
+  SubsequentDelim SD2(" ");
+  S = SD2;
+  EXPECT_EQ(S, "");
+  S = SD2;
+  EXPECT_EQ(S, " ");
+}