[PATCH] D20138: Refactor CodeView TypeRecord visitors to use common structures

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:74
@@ +73,3 @@
+  StringRef Rest;
+  std::tie(Str, Rest) = getBytesAsCharacters(Data).split('\0');
+  Data = ArrayRef<uint8_t>(Rest.bytes_begin(), Rest.bytes_end());
----------------
Given that you wired up error_code, this should return an error if Data.size() == Str.size(), which implies that there was no null terminator.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:83
@@ +82,3 @@
+  uint32_t Size = sizeof(T) * N;
+  if (Data.size() < N)
+    return std::make_error_code(std::errc::illegal_byte_sequence);
----------------
I think you meant `Data.size() < Size` :)

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:274
@@ -85,1 +273,3 @@
 
+  static ErrorOr<MemberFunctionRecord> deserialize(TypeRecordKind Kind,
+                                                   ArrayRef<uint8_t> &Data) {
----------------
We probably shouldn't inline all these deserialization methods. We should probably make a TypeRecord.cpp for these.

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:629
@@ +628,3 @@
+    if (Props & uint16_t(ClassOptions::HasUniqueName)) {
+      UniqueName = StringRef(reinterpret_cast<const char *>(Data.data()));
+      Data = Data.slice(UniqueName.size() + 1);
----------------
consumeCString

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:695
@@ +694,3 @@
+    if (Props & uint16_t(ClassOptions::HasUniqueName)) {
+      UniqueName = StringRef(reinterpret_cast<const char *>(Data.data()));
+      Data = Data.slice(UniqueName.size() + 1);
----------------
consumeCString

================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:792
@@ +791,3 @@
+    std::vector<VirtualTableSlotKind> Slots;
+    for (int I = 0; I < L->VFEntryCount; ++I) {
+      if (Data.empty())
----------------
> You should check up front that there are at least (VFEntryCount +1) / 2 bytes in Data, or the slices will abort.

This comment is still relevant. Maybe you could handle it by doing this:
  ArrayRef<uint8_t> SlotBytes;
  if (auto EC = consumeArray(SlotBytes, Data, (L->VFEntryCount + 1) / 2))
    return EC;
  if (L->VFEntryCount % 2 == 1)
    SlotBytes = SlotBytes.drop_back(1); // plus other special handling for that last byte
  for (uint8_t Byte : SlotBytes) {
    // handle two nibbles at a time
  }


http://reviews.llvm.org/D20138