[PATCH] D114223: Make ScopedPrinter interface virtual

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:230-232
+    for (const auto &Item : List) {
+      StringList.emplace_back(to_string(Item));
+    }
----------------
No braces for single-line ifs.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:351
+  virtual void arrayBegin() {
+    startLine() << "[\n";
+    indent();
----------------
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().
```


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:381
   template <typename S, typename T, typename... TArgs>
-  void printVersionInternal(S Value, T Value2, TArgs... Args) {
+  void printVersionImpl(S Value, T Value2, TArgs... Args) {
     getOStream() << Value << ".";
----------------
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).


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:386-387
 
-  template <typename T>
-  static bool flagName(const EnumEntry<T> &lhs, const EnumEntry<T> &rhs) {
+  static bool flagName(const FlagEntry &lhs, const FlagEntry &rhs) {
     return lhs.Name < rhs.Name;
   }
----------------
Up to you, but I'd probably fix the case of the variables in this patch.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:397-399
+    for (const auto &Flag : Flags) {
+      startLine() << "  " << Flag.Name << " (" << hex(Flag.Value) << ")\n";
+    }
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114223/new/

https://reviews.llvm.org/D114223