[PATCH] D40518: [CodeView] Re-write TypeSerializer and TypeTableBuilder

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/ContinuationRecordBuilder.h:55
+
+  template <typename RecordType> void writeMemberType(RecordType &Record);
+
----------------
I see how this is supposed to work, but maybe stamping out overloads for all of the member types in the header and redirecting them to use a common template implementation in the .cpp file is a better way to do this.

Compilers sometimes warn when an implicit template instantiation is requested from a function template declaration with no body.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/SimpleTypeSerializer.h:41
+
+  template <typename T> ArrayRef<uint8_t> serialize(T &Record);
+
----------------
Ditto, re: stamping out overloads.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeTableBuilder.h:1-2
-//===- TypeTableBuilder.h ---------------------------------------*- C++ -*-===//
+//===- TypeTableBuilder.h -----------------------------------------*- C++
+//-*-===//
 //
----------------
Fix that so it's on one line.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeTableBuilder.h:11
 
-#ifndef LLVM_DEBUGINFO_CODEVIEW_TYPETABLEBUILDER_H
-#define LLVM_DEBUGINFO_CODEVIEW_TYPETABLEBUILDER_H
+#ifndef LLVM_DEBUGINFO_CODEVIEW_TYPESERIALIZER_H
+#define LLVM_DEBUGINFO_CODEVIEW_TYPESERIALIZER_H
----------------
Fix the header guard macro name


================
Comment at: llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp:8
+struct ContinuationRecord {
+  ulittle16_t Kind{uint16_t(TypeLeafKind::LF_INDEX)};
+  ulittle16_t Size{0};
----------------
I hate unicorn initialization =P


================
Comment at: llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp:108
+    // We need to inject some bytes before the member we just wrote but after
+    // the
+    // previous member.  Save off the length of the member we just wrote so that
----------------
nit: reflow


================
Comment at: llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp:114
+    // Since this member now becomes a new top-level record, it should have
+    // gotten
+    // a RecordPrefix injected, and that RecordPrefix + the member we just wrote
----------------
nit: reflow


https://reviews.llvm.org/D40518