[PATCH] D20876: [PDB] Print section contributions and section map

[Zachary Turner via llvm-commits]

zturner added inline comments.

================
Comment at: tools/llvm-pdbdump/llvm-pdbdump.cpp:509
@@ -507,1 +508,3 @@
 
+static Error dumpSectionContrs(ScopedPrinter &P, PDBFile &File) {
+  if (!opts::DumpSectionContrs)
----------------
ruiu wrote:
> I'd do s/contr/contrib/g to all files in this patch.
A little tricky since that will catch Contrib too, but anyway I'll fix up the rest of the instances.

================
Comment at: tools/llvm-pdbdump/llvm-pdbdump.cpp:518
@@ +517,3 @@
+  ListScope L(P, "Section Contributions");
+  class Visitor : public ISectionContribVisitor {
+  public:
----------------
ruiu wrote:
> Define this class outside of this function.
> 
> Also this class definition made me think of the visitor design choice. Currently visitor's callback is a class with visit() member function. But it can be just a std::function, no? We can create a lambda here and pass it to visitSectionContributions function.
The problem is we don't know whether it will be called back with a `SectionContrib` or a `SectionContrib2`.  If it were possible to make a "multi lambda" or something that supported overloads, then yes we could do that.  Of course, I could raise the logic outside and use a switch statement, but in general I like the pattern of handling variant-data with a visitor.

Is there a reason to prefer defining the class outside the function?  This is effectively a lambda with an overload, so just like your lambda would be scoped to the function, I wrote the class the same way.

================
Comment at: tools/llvm-pdbdump/llvm-pdbdump.cpp:539
@@ +538,3 @@
+      P.printNumber("Reloc CRC", SC.RelocCrc);
+      P.flush();
+    }
----------------
ruiu wrote:
> Why do you have to call flush?
Not strictly necessary, other dump functions do this as well, I guess because it's nice to be able to see the output in a debugger if you're stepping through


http://reviews.llvm.org/D20876