[PATCH] D31832: Improves pretty printing of variable types in llvm-pdbdump

[Zachary Turner via Phabricator via llvm-commits]

zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

lgtm, only some minor nits.



================
Comment at: llvm/tools/llvm-pdbdump/PrettyVariableDumper.cpp:154
+  PointeeType->dump(*this);
+  if (auto Func = dyn_cast<PDBSymbolTypeFunctionSig>(PointeeType.get())) {
+    // A hack to get the calling convention in the right spot.
----------------
You'll probably need to do a merge here, as I changed the syntax for this line.


================
Comment at: llvm/tools/llvm-pdbdump/PrettyVariableDumper.cpp:159
+    WithColor(Printer, PDB_ColorItem::Keyword).get() << CC << " ";
+  } else if (dyn_cast<PDBSymbolTypeArray>(PointeeType.get())) {
+    Printer << " (";
----------------
here I'd suggest just using `isa<PDBSymbolTypeArray>` since we don't actually need the derived class.


================
Comment at: llvm/tools/llvm-pdbdump/PrettyVariableDumper.cpp:170-172
+  auto PointeeType = Symbol.getPointeeType();
+  if (!PointeeType)
+    return;
----------------
I wonder if it's worth asserting here.  You can still return if it's null, but it seems like a bad PDB file at the same time.


================
Comment at: llvm/tools/llvm-pdbdump/PrettyVariableDumper.cpp:173-174
+    return;
+  if (dyn_cast<PDBSymbolTypeFunctionSig>(PointeeType.get()) ||
+      dyn_cast<PDBSymbolTypeArray>(PointeeType.get())) {
+    Printer << ")";
----------------
`isa<>` again.


https://reviews.llvm.org/D31832