Make ScopedPrinter interface virtual

[Jaysonyan]

Summary

Make the interface provided by ScopedPrinter virtual to support a future JSONScopedPrinter which will supply it's own implementation.

This change is split off from D114052.

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

232–234

No braces for single-line ifs.

349

You could avoid the logic duplication of arrayBegin with objectBegin (and same for the other new functions), with helper functions, private to the ScopedPrinter class, e.g:

virtual void objectBegin() {
  scopedBegin('{');
}

virtual void arrayBegin() {
  scopedBegin('[');
}

void scopedBegin(char Symbol) {
  unindent();
  startLine() << Symbol << "\n";
}

// Same again for scopedBegin(StringRef Label, char Symbol) and scopedEnd().

355

This ones a bit different, and can probably be left as printVersionInternal, if you want, as it's not part of the virtual interface (I also have no objections about the new name though).

359–391

Up to you, but I'd probably fix the case of the variables in this patch.

401–403

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

97

And check clang-format's output here too.

[Jaysonyan]

Add abstraction to object/array begin/end.
Misc formatting and function renaming.

[jhenderson]

LGTM, but don't submit unless the other patches in the stack are approved too.

[Jaysonyan]

Make printFlags and printObject virtual.

[Jaysonyan]

Misc formatting.

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

165–166

I've just remembered that it's common practice to leave the explicit llvm:: prefix on cases where the function is one of the standard algorithm-like functions, so it's probably best to keep it here.

406

I'm not a fan of this function name. In some ways, it's less raw than the regular printFlags code, as it includes names. Any particular need for it to be a different name, or could we just make use of function overloading?

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

272–278

FWIW, these two functions don't need to be in the interface, since they should implicitly convert to StringRef quite happily. Perhaps a separate patch to remove them?

280–283

On the note of separate patches, I'd be inclined to move this to be with the other printNumber overloads.

[Jaysonyan]

Updated printRawFlagsImpl to be overloaded printFlagsImpl method.
Updated formatting.

[Jaysonyan]

llvm/include/llvm/Support/ScopedPrinter.h

406

I've updated printRawFlagsImpl to share the printFlagsImpl name and take a list of HexNumbers instead of FlagEntry.

[jhenderson]

LGTM again.

[Jaysonyan]

Add printList overloads (to handle future JSONScopedPrinter number and boolean printing).

[Jaysonyan]

Update printList for uint8_t and int8_t and remove ArrayRef from printListImpl(...)

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

258

I don't think you need the explicit unsigned conversion, right?

277

Ditto (for the int conversion).

[Jaysonyan]

Remove unneeded explicit cast in printList(...).

[Jaysonyan]

llvm/include/llvm/Support/ScopedPrinter.h

258

You're right, removed the conversion. Thanks for the catch.

[jhenderson]

LGTM again!

[jhenderson]

llvm/include/llvm/Support/ScopedPrinter.h

214–216

Do we need an equivalent printList virtual method for APSInt?

[Jaysonyan]

Add printList implementation for APSInt.

[jhenderson]

LGTM again!

llvm/include/llvm/Support/ScopedPrinter.h

struct HexNumber {
   HexNumber(unsigned char Value) : Value(Value) {}
   HexNumber(unsigned short Value) : Value(Value) {}
   HexNumber(unsigned int Value) : Value(Value) {}
   HexNumber(unsigned long Value) : Value(Value) {}
   HexNumber(unsigned long long Value) : Value(Value) {}
   uint64_t Value;
 };
 
+struct FlagEntry {
+  FlagEntry(StringRef Name, char Value)
+      : Name(Name), Value(static_cast<unsigned char>(Value)) {}
+  FlagEntry(StringRef Name, signed char Value)
+      : Name(Name), Value(static_cast<unsigned char>(Value)) {}
+  FlagEntry(StringRef Name, signed short Value)
+      : Name(Name), Value(static_cast<unsigned short>(Value)) {}
+  FlagEntry(StringRef Name, signed int Value)
+      : Name(Name), Value(static_cast<unsigned int>(Value)) {}
+  FlagEntry(StringRef Name, signed long Value)
+      : Name(Name), Value(static_cast<unsigned long>(Value)) {}
+  FlagEntry(StringRef Name, signed long long Value)
+      : Name(Name), Value(static_cast<unsigned long long>(Value)) {}
+  FlagEntry(StringRef Name, unsigned char Value) : Name(Name), Value(Value) {}
+  FlagEntry(StringRef Name, unsigned short Value) : Name(Name), Value(Value) {}
+  FlagEntry(StringRef Name, unsigned int Value) : Name(Name), Value(Value) {}
+  FlagEntry(StringRef Name, unsigned long Value) : Name(Name), Value(Value) {}
+  FlagEntry(StringRef Name, unsigned long long Value)
+      : Name(Name), Value(Value) {}
+  StringRef Name;
+  uint64_t Value;
+};
+
 raw_ostream &operator<<(raw_ostream &OS, const HexNumber &Value);
 std::string to_hexString(uint64_t Value, bool UpperCase = true);
 
 template <class T> std::string to_string(const T &Value) {
   std::string number;
   llvm::raw_string_ostream stream(number);
   stream << Value;
   return stream.str();
 }
 
 class ScopedPrinter {
 public:
   ScopedPrinter(raw_ostream &OS) : OS(OS), IndentLevel(0) {}
 
+  virtual ~ScopedPrinter() {}
+
   void flush() { OS.flush(); }
 
   void indent(int Levels = 1) { IndentLevel += Levels; }
 
   void unindent(int Levels = 1) {
     IndentLevel = std::max(0, IndentLevel - Levels);
   }
 
for (const auto &EnumItem : EnumValues) {
       if (EnumItem.Value == Value) {
         Name = EnumItem.Name;
         Found = true;
         break;
       }
     }
 
     if (Found) {
-      startLine() << Label << ": " << Name << " (" << hex(Value) << ")\n";
+      printHex(Label, Name, Value);
     } else {
-      startLine() << Label << ": " << hex(Value) << "\n";
+      printHex(Label, Value);
     }
   }
 
   template <typename T, typename TFlag>
   void printFlags(StringRef Label, T Value, ArrayRef<EnumEntry<TFlag>> Flags,
                   TFlag EnumMask1 = {}, TFlag EnumMask2 = {},
                   TFlag EnumMask3 = {}) {
-    typedef EnumEntry<TFlag> FlagEntry;
-    typedef SmallVector<FlagEntry, 10> FlagVector;
-    FlagVector SetFlags;
+    SmallVector<FlagEntry, 10> SetFlags;
 
     for (const auto &Flag : Flags) {
       if (Flag.Value == 0)
         continue;
 
       TFlag EnumMask{};
       if (Flag.Value & EnumMask1)
         EnumMask = EnumMask1;
       else if (Flag.Value & EnumMask2)
         EnumMask = EnumMask2;
       else if (Flag.Value & EnumMask3)
         EnumMask = EnumMask3;
       bool IsEnum = (Flag.Value & EnumMask) != 0;
       if ((!IsEnum && (Value & Flag.Value) == Flag.Value) ||
           (IsEnum && (Value & EnumMask) == Flag.Value)) {
-        SetFlags.push_back(Flag);
+        SetFlags.emplace_back(Flag.Name, Flag.Value);
       }
     }
 
-    llvm::sort(SetFlags, &flagName<TFlag>);
-
-    startLine() << Label << " [ (" << hex(Value) << ")\n";
-    for (const auto &Flag : SetFlags) {
-      startLine() << "  " << Flag.Name << " (" << hex(Flag.Value) << ")\n";
-    }
-    startLine() << "]\n";
+    llvm::sort(SetFlags, &flagName);
+    printFlagsImpl(Label, hex(Value), SetFlags);
   }
 
   template <typename T> void printFlags(StringRef Label, T Value) {
     startLine() << Label << " [ (" << hex(Value) << ")\n";
     uint64_t Flag = 1;
     uint64_t Curr = Value;
     while (Curr > 0) {
       if (Curr & 1)
         startLine() << "  " << hex(Flag) << "\n";
       Curr >>= 1;
       Flag <<= 1;
     }
     startLine() << "]\n";
   }
 
-  void printNumber(StringRef Label, uint64_t Value) {
+  virtual void printNumber(StringRef Label, uint64_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printNumber(StringRef Label, uint32_t Value) {
+  virtual void printNumber(StringRef Label, uint32_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printNumber(StringRef Label, uint16_t Value) {
+  virtual void printNumber(StringRef Label, uint16_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printNumber(StringRef Label, uint8_t Value) {
+  virtual void printNumber(StringRef Label, uint8_t Value) {
     startLine() << Label << ": " << unsigned(Value) << "\n";
   }
 
-  void printNumber(StringRef Label, int64_t Value) {
+  virtual void printNumber(StringRef Label, int64_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printNumber(StringRef Label, int32_t Value) {
+  virtual void printNumber(StringRef Label, int32_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printNumber(StringRef Label, int16_t Value) {
+  virtual void printNumber(StringRef Label, int16_t Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printNumber(StringRef Label, int8_t Value) {
+  virtual void printNumber(StringRef Label, int8_t Value) {
     startLine() << Label << ": " << int(Value) << "\n";
   }
 
-  void printNumber(StringRef Label, const APSInt &Value) {
+  virtual void printNumber(StringRef Label, const APSInt &Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  void printBoolean(StringRef Label, bool Value) {
+  virtual void printBoolean(StringRef Label, bool Value) {
     startLine() << Label << ": " << (Value ? "Yes" : "No") << '\n';
   }
 
   template <typename... T> void printVersion(StringRef Label, T... Version) {
     startLine() << Label << ": ";
     printVersionInternal(Version...);
     getOStream() << "\n";
   }
 
   template <typename T> void printList(StringRef Label, const T &List) {
-    startLine() << Label << ": [";
-    ListSeparator LS;
+    SmallVector<std::string, 10> StringList;
     for (const auto &Item : List)
-      OS << LS << Item;
-    OS << "]\n";
+      StringList.emplace_back(to_string(Item));
+    printListImpl(Label, StringList);
   }
 
   template <typename T, typename U>
   void printList(StringRef Label, const T &List, const U &Printer) {
     startLine() << Label << ": [";
     ListSeparator LS;
     for (const auto &Item : List) {
       OS << LS;
       Printer(OS, Item);
     }
     OS << "]\n";
   }
 
   template <typename T> void printHexList(StringRef Label, const T &List) {
-    startLine() << Label << ": [";
-    ListSeparator LS;
+    SmallVector<HexNumber> HexList;
     for (const auto &Item : List)
-      OS << LS << hex(Item);
-    OS << "]\n";
+      HexList.emplace_back(Item);
+    printHexListImpl(Label, HexList);
   }
 
   template <typename T> void printHex(StringRef Label, T Value) {
-    startLine() << Label << ": " << hex(Value) << "\n";
+    printHexImpl(Label, hex(Value));
   }
 
   template <typename T> void printHex(StringRef Label, StringRef Str, T Value) {
-    startLine() << Label << ": " << Str << " (" << hex(Value) << ")\n";
+    printHexImpl(Label, Str, hex(Value));
   }
 
   template <typename T>
   void printSymbolOffset(StringRef Label, StringRef Symbol, T Value) {
-    startLine() << Label << ": " << Symbol << '+' << hex(Value) << '\n';
+    printSymbolOffsetImpl(Label, Symbol, hex(Value));
   }
 
-  void printString(StringRef Value) { startLine() << Value << "\n"; }
+  virtual void printString(StringRef Value) { startLine() << Value << "\n"; }
 
-  void printString(StringRef Label, StringRef Value) {
+  virtual void printString(StringRef Label, StringRef Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
   void printString(StringRef Label, const std::string &Value) {
     printString(Label, StringRef(Value));
   }
 
-  void printString(StringRef Label, const char* Value) {
+  void printString(StringRef Label, const char *Value) {
     printString(Label, StringRef(Value));
   }
 
   template <typename T>
   void printNumber(StringRef Label, StringRef Str, T Value) {
-    startLine() << Label << ": " << Str << " (" << Value << ")\n";
+    printNumberImpl(Label, Str, to_string(Value));
   }
 
   void printBinary(StringRef Label, StringRef Str, ArrayRef<uint8_t> Value) {
     printBinaryImpl(Label, Str, Value, false);
   }
 
   void printBinary(StringRef Label, StringRef Str, ArrayRef<char> Value) {
     auto V = makeArrayRef(reinterpret_cast<const uint8_t *>(Value.data()),
                           Value.size());
auto V = makeArrayRef(reinterpret_cast<const uint8_t *>(Value.data()),
                           Value.size());
     printBinaryImpl(Label, StringRef(), V, true);
   }
 
   template <typename T> void printObject(StringRef Label, const T &Value) {
     startLine() << Label << ": " << Value << "\n";
   }
 
-  raw_ostream &startLine() {
+  virtual void objectBegin() { scopedBegin('{'); }
+
+  virtual void objectBegin(StringRef Label) { scopedBegin(Label, '{'); }
+
+  virtual void objectEnd() { scopedEnd('}'); }
+
+  virtual void arrayBegin() { scopedBegin('['); }
+
+  virtual void arrayBegin(StringRef Label) { scopedBegin(Label, '['); }
+
+  virtual void arrayEnd() { scopedEnd(']'); }
+
+  virtual raw_ostream &startLine() {
     printIndent();
     return OS;
   }
 
-  raw_ostream &getOStream() { return OS; }
+  virtual raw_ostream &getOStream() { return OS; }
 
 private:
   template <typename T> void printVersionInternal(T Value) {
     getOStream() << Value;
   }
 
   template <typename S, typename T, typename... TArgs>
   void printVersionInternal(S Value, T Value2, TArgs... Args) {
     getOStream() << Value << ".";
     printVersionInternal(Value2, Args...);
   }
 
-  template <typename T>
-  static bool flagName(const EnumEntry<T> &lhs, const EnumEntry<T> &rhs) {
-    return lhs.Name < rhs.Name;
+  static bool flagName(const FlagEntry &LHS, const FlagEntry &RHS) {
+    return LHS.Name < RHS.Name;
   }
 
-  void printBinaryImpl(StringRef Label, StringRef Str, ArrayRef<uint8_t> Value,
-                       bool Block, uint32_t StartOffset = 0);
+  virtual void printBinaryImpl(StringRef Label, StringRef Str,
+                               ArrayRef<uint8_t> Value, bool Block,
+                               uint32_t StartOffset = 0);
+
+  virtual void printFlagsImpl(StringRef Label, HexNumber Value,
+                              ArrayRef<FlagEntry> Flags) {
+    startLine() << Label << " [ (" << Value << ")\n";
+    for (const auto &Flag : Flags)
+      startLine() << "  " << Flag.Name << " (" << hex(Flag.Value) << ")\n";
+    startLine() << "]\n";
+  }
+
+  virtual void printListImpl(StringRef Label,
+                             const ArrayRef<std::string> List) {
+    startLine() << Label << ": [";
+    ListSeparator LS;
+    for (const auto &Item : List)
+      OS << LS << Item;
+    OS << "]\n";
+  }
+
+  virtual void printHexListImpl(StringRef Label,
+                                const ArrayRef<HexNumber> List) {
+    startLine() << Label << ": [";
+    ListSeparator LS;
+    for (const auto &Item : List)
+      OS << LS << hex(Item);
+    OS << "]\n";
+  }
+
+  virtual void printHexImpl(StringRef Label, HexNumber Value) {
+    startLine() << Label << ": " << Value << "\n";
+  }
+
+  virtual void printHexImpl(StringRef Label, StringRef Str, HexNumber Value) {
+    startLine() << Label << ": " << Str << " (" << Value << ")\n";
+  }
+
+  virtual void printSymbolOffsetImpl(StringRef Label, StringRef Symbol,
+                                     HexNumber Value) {
+    startLine() << Label << ": " << Symbol << '+' << Value << '\n';
+  }
+
+  virtual void printNumberImpl(StringRef Label, StringRef Str,
+                               StringRef Value) {
+    startLine() << Label << ": " << Str << " (" << Value << ")\n";
+  }
+
+  void scopedBegin(char Symbol) {
+    startLine() << Symbol << '\n';
+    indent();
+  }
+
+  void scopedBegin(StringRef Label, char Symbol) {
+    startLine() << Label;
+    if (!Label.empty())
+      OS << ' ';
+    OS << Symbol << '\n';
+    indent();
+  }
+
+  void scopedEnd(char Symbol) {
+    unindent();
+    startLine() << Symbol << '\n';
+  }
 
   raw_ostream &OS;
   int IndentLevel;
   StringRef Prefix;
 };
 
 template <>
 inline void
 ScopedPrinter::printHex<support::ulittle16_t>(StringRef Label,
                                               support::ulittle16_t Value) {
   startLine() << Label << ": " << hex(Value) << "\n";
 }
 
-template<char Open, char Close>
 struct DelimitedScope {
-  explicit DelimitedScope(ScopedPrinter &W) : W(W) {
-    W.startLine() << Open << '\n';
-    W.indent();
-  }
-
-  DelimitedScope(ScopedPrinter &W, StringRef N) : W(W) {
-    W.startLine() << N;
-    if (!N.empty())
-      W.getOStream() << ' ';
-    W.getOStream() << Open << '\n';
-    W.indent();
-  }
-
-  ~DelimitedScope() {
-    W.unindent();
-    W.startLine() << Close << '\n';
+  DelimitedScope(ScopedPrinter &W) : W(W) {}
+  virtual ~DelimitedScope(){};
+  ScopedPrinter &W;
+};
+
+struct DictScope : DelimitedScope {
+  explicit DictScope(ScopedPrinter &W) : DelimitedScope(W) { W.objectBegin(); }
+
+  DictScope(ScopedPrinter &W, StringRef N) : DelimitedScope(W) {
+    W.objectBegin(N);
   }
 
-  ScopedPrinter &W;
+  ~DictScope() { W.objectEnd(); }
 };
 
-using DictScope = DelimitedScope<'{', '}'>;
-using ListScope = DelimitedScope<'[', ']'>;
+struct ListScope : DelimitedScope {
+  explicit ListScope(ScopedPrinter &W) : DelimitedScope(W) { W.arrayBegin(); }
+
+  ListScope(ScopedPrinter &W, StringRef N) : DelimitedScope(W) {
+    W.arrayBegin(N);
+  }
+
+  ~ListScope() { W.arrayEnd(); }
+};
 
 } // namespace llvm
 
 #endif