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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

I'm satisfied, but you might want to wait to hear from at least one other reviewer.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:30
+
+namespace llvm {
+template <> struct VarStreamArrayExtractor<codeview::FileChecksumEntry> {
----------------
zturner wrote:
> 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.
I'm fine with it.  I just noticed that at least one other file in this patch doesn't do this.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ModuleDebugFileChecksumFragment.h:36
+  static Error extract(BinaryStreamRef Stream, uint32_t &Len,
+                       codeview::FileChecksumEntry &Item, void *Ctx);
+};
----------------
zturner wrote:
> 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.
I was just going for consistency (see `LineColumnExtractor`), and because it makes the point of the `ContextType` typedef obvious.


https://reviews.llvm.org/D32547