[PATCH] D20936: [pdb] Parse module line info
Adrian McCarthy via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 2 16:33:30 PDT 2016
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
More information about the llvm-commits
mailing list