[PATCH] D20936: [pdb] Parse module line info

[Adrian McCarthy via llvm-commits]

amccarth added a comment.

DebugInfo/CodeView/ModuleSubstreamVisitor.cpp probably shouldn't be empty (or it shouldn't be added).


================
Comment at: include/llvm/DebugInfo/CodeView/CodeView.h:16
@@ -15,1 +15,3 @@
 
+#include "llvm/Support/Endian.h"
+
----------------
ruiu wrote:
> Instead of adding an #include to this header, can you include it into .cpp files that actually need Endian.h?
Nothing in this file actually uses the endian types (as far as I can see).  I'd include Endian.h in llvm/DebugInfo/CodeView/ModuleSubstream.h and ModuleSubstreamVisitor.h.

================
Comment at: include/llvm/DebugInfo/CodeView/StreamArray.h:55
@@ -54,2 +54,3 @@
   VarStreamArray() {}
+  VarStreamArray(const Extractor &E) : E(E) {}
 
----------------
Should this be explicit?

================
Comment at: include/llvm/DebugInfo/CodeView/StreamArray.h:214
@@ +213,3 @@
+    if (Array.size() <= Index)
+      this->Index = uint32_t(-1);
+  }
----------------
`this->Index` to distinguish from the `Index` parameter?  Ug.  Can you give the parameter a distinct name?

================
Comment at: include/llvm/DebugInfo/CodeView/StreamReader.h:56
@@ -57,1 +55,3 @@
+  template <typename T, typename U>
+  Error readArray(VarStreamArray<T, U> &Array, uint32_t Size) {
     StreamRef S;
----------------
Why `U` and not `Extractor`?

================
Comment at: tools/llvm-pdbdump/llvm-pdbdump.cpp:576
@@ +575,3 @@
+          RecordVisitor V(P);
+          for (auto &L : ModS.lines(&HadError)) {
+            if (auto EC = codeview::visitModuleSubstream(L, V))
----------------
Should(could) `L` be a `const auto &`?


http://reviews.llvm.org/D20936