[PATCH] D121421: [VFS] Add dump to the whole FileSystem hierarchy

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

This is awesome! Maybe eventually we'll want more details about some of these but this is enough to be useful.

A few comments inline, but besides the rename I suggested from dump to print, up to you whether to address now or later. It'd also be nice to unit test `print()` for OverlayFileSystem; in which case you might want the extra flags I suggest inline.



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:310
+
+  virtual void dump(raw_ostream &OS) const {}
+
----------------
Usually, the dump that takes a `raw_ostream` is called `print()`. Can we follow that here too?

I also think it'd be useful to have it take a second argument for whether to print recursively. A bit awkward with virtual functions, but you can make it work nicely with indirection:
```
lang=c++
  void print(raw_ostream &OS, bool PrintContents = true, bool PrintRecursively = false) {
    printImpl(OS, PrintRecursively);
  }
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
  LLVM_DUMP_METHOD void dump() const; // { print(dbgs(), /*PrintContents=*/true, /*PrintRecursively=*/true); }
#endif

protected:
  virtual void printImpl(raw_ostream &OS, bool, bool) {}
```


================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:929-930
 
-  void dump(raw_ostream &OS) const;
+  void dump(raw_ostream &OS) const override;
   void dumpEntry(raw_ostream &OS, Entry *E, int NumSpaces = 0) const;
 };
----------------
I'd suggest renaming both of these in a prep commit (you probably don't need pre-commit review for that... as long as tests pass):
- `print()`
- `printEntry()`

and reserve `dump()` for when there's no argument. I think this better matches usage elsewhere in LLVM.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:155
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+LLVM_DUMP_METHOD void FileSystem::dump() const { dump(dbgs()); }
+#endif
----------------
I don't think you need to repeat `LLVM_DUMP_METHOD` here. Either header or source is sufficient.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:478-483
+  OS << "OverlayFileSystem running operations on: \n";
+  for (auto FS : enumerate(overlays_range())) {
+    OS << FS.index() << ") ";
+    FS.value()->dump(OS);
+    OS << "\n";
+  }
----------------
I'd suggest (with the new flags I suggested):
```
lang=c++
void OverlayFileSystem::printImpl(raw_ostream &OS, bool PrintContents, bool PrintRecursively) const {
  OS << "OverlayFileSystem:";
  if (!PrintContents) {
    OS << "\n";
    return;
  }

  for (auto FS : enumerate(overlays_range())) {
    OS << FS.index() << ") ";
    FS.value()->print(OS, /*PrintContents=*/PrintRecursively, PrintRecursively);
    OS << "\n";
  }
}
```
... or something... maybe the flags aren't quite right, but the idea is that `print(OS, true, false)` would print:
```
OverlayFileSystem:
FS1
```
without printing the contents of FS1.

Also makes me feel like an `Indent` would be useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121421