[PATCH] D26253: [CodeView] Hook CodeViewRecordIO up to the type serialization path.

[Adrian McCarthy via llvm-commits]

amccarth added a comment.

Looks pretty good.



================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecordMapping.h:22
+class StreamWriter;
+}
 namespace codeview {
----------------
These forward declarations seem unnecessary.  The TypeRecordMapping constructors are defined in this header, so I believe you need the complete definitions of msf::StreamReader and msf::StreamWriter, and you have the complete definitions, because CodeViewRecordIO.h includes them transitively (because it needs the complete types).


================
Comment at: include/llvm/DebugInfo/CodeView/TypeSerializer.h:22
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/iterator_range.h"
+#include "llvm/Support/Allocator.h"
----------------
I guess clang-format puts includes in ASCII order rather than alphabetical order since `'S' < 'i'`.


================
Comment at: include/llvm/DebugInfo/CodeView/TypeSerializer.h:25
+#include "llvm/Support/Error.h"
+
+namespace llvm {
----------------
You probably should `#include <string.h>` for `::memcpy` and `::memmove`, which are used several times in this header.

(or `<cstring>` for `std::memcpy` and `std::memmove`)


================
Comment at: include/llvm/DebugInfo/CodeView/TypeTableBuilder.h:68
+
+    for (auto Record : Serializer.records()) {
+      Func(TypeIndex(Index), Record);
----------------
Should `Record` be a reference or are you intentionally applying Func to copies of the records?  Oh, I see.  These aren't actually records but ArrayRefs to records, so the copy is just a couple pointers.  Some days I really miss Hungarian notation.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1579
   } else {
-    FieldListRecordBuilder Fields;
+    FieldListRecordBuilder FLBR(TypeTable);
+
----------------
I think you meant `FLRB` rather than `FLBR`.

It's too bad the FieldListRecordBuilder c'tor and d'tor don't do the begin and end calls for you.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1786
   ClassInfo Info = collectClassInfo(Ty);
-  FieldListRecordBuilder Fields;
+  FieldListRecordBuilder FLBR(TypeTable);
+  FLBR.begin();
----------------
`FLRB`


https://reviews.llvm.org/D26253