[PATCH] D32547: [CodeView] Isolate the parsing code for various types of CV debug info fragments

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

Looks nice.  I noticed just minor stuff.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:1
+//===- ModuleDebugLineFragment.h --------------------------------*- C++ -*-===//
+//
----------------
`ModuleDebugFileChecksumFragment.h`


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:30
+
+namespace llvm {
+template <> struct VarStreamArrayExtractor<codeview::FileChecksumEntry> {
----------------
It seems weird to close the llvm namespace, re-open it, close it again, open it again...  Is this an intentional style decision?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:36
+  static Error extract(BinaryStreamRef Stream, uint32_t &Len,
+                       codeview::FileChecksumEntry &Item, void *Ctx);
+};
----------------
Should that last parameter be `ContextType *Ctx`?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFragment.h:14
 #include "llvm/DebugInfo/CodeView/CodeView.h"
-#include "llvm/Support/BinaryStreamArray.h"
-#include "llvm/Support/BinaryStreamRef.h"
-#include "llvm/Support/Endian.h"
-#include "llvm/Support/Error.h"
+#include "llvm/Support/Casting.h"
 
----------------
What's `Casting.h` for in this file?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h:1
+//===- ModuleDebugFragment.h ------------------------------------*- C++ -*-===//
+//
----------------
`ModuleDebugFragmentRecord.h`


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFragmentRecord.h:53
+  static Error extract(BinaryStreamRef Stream, uint32_t &Length,
+                       codeview::ModuleDebugFragmentRecord &Info, void *Ctx) {
+    if (auto EC = codeview::ModuleDebugFragmentRecord::initialize(Stream, Info))
----------------
`ContextType *Ctx`?


================
Comment at: llvm/lib/DebugInfo/CodeView/ModuleDebugLineFragment.cpp:2
+//===- ModuleDebugLineFragment.cpp --------------------------------*- C++
+//-*-===//
+//
----------------
Bad wrap.


================
Comment at: llvm/lib/DebugInfo/CodeView/ModuleDebugLineFragment.cpp:34
+                                     "Invalid line block record size");
+  uint32_t Size = BlockHeader->BlockSize - sizeof(LineBlockFragmentHeader);
+  if (LineInfoSize > Size)
----------------
`Size`, `LineInfoSize`, and `HasColumn` could all be `const`, which might be nice since you can't scope their lifetimes any tighter.


================
Comment at: llvm/lib/DebugInfo/CodeView/ModuleDebugUnknownFragment.cpp:11
+#include "llvm/DebugInfo/CodeView/ModuleDebugUnknownFragment.h"
\ No newline at end of file

----------------
No newline at end of file


https://reviews.llvm.org/D32547