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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:30
+
+namespace llvm {
+template <> struct VarStreamArrayExtractor<codeview::FileChecksumEntry> {
----------------
amccarth wrote:
> It seems weird to close the llvm namespace, re-open it, close it again, open it again...  Is this an intentional style decision?
This class can't go in the codeview namespace, only the llvm namespace.  When multiple levels of namespace nesting are involved, I often find it difficult to follow which one I'm looking at if I partially close and reopen at various points in the file.  So I did it this way so that anyone looking at this block of code can immediately figure out the entire namespace hierarchy.  In any case, I'm not strongly tied to it, so I can change it.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:36
+  static Error extract(BinaryStreamRef Stream, uint32_t &Len,
+                       codeview::FileChecksumEntry &Item, void *Ctx);
+};
----------------
amccarth wrote:
> Should that last parameter be `ContextType *Ctx`?
No reason it couldn't be, mostly just that it was a bit more to type.


================
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"
 
----------------
amccarth wrote:
> What's `Casting.h` for in this file?
Since this entire hierarchy is designed to work with the llvm casting operators, I included it here so that no matter which subclass's header file you include, you always have access to the casting operators.  I could leave it up to the individual header files, this is just a little convenience for the users.


https://reviews.llvm.org/D32547