[PATCH] D33564: [CodeView Type Merging] Make a Type Index Iterator that bypasses the visitor framework

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Sweet, the minimal thing. :)



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeIndexIterator.h:1
+//===- TypeIndexIterator.h --------------------------------------*- C++ -*-===//
+//
----------------
This basically builds a SmallVector. Its... sort of like an iterator. Do we still want to call it one? I feel like "Iterator" has a specific meaning in C++, and this isn't it. Maybe `TypeIndexFinder.h` to go with `findTypeIndexOffsets` or something?

Alternatively, do you want to just mush all this functionality into `TypeSerializer.h/cpp`?


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeRecord.h:288
+                PointerOptions PO, uint8_t Size,
+                TypeIndex MemberPtrContainingType)
       : TypeRecord(TypeRecordKind::Pointer), ReferentType(ReferentType),
----------------
How is this new constructor used? It's reasonable to not force the caller to construct a MemberPointerInfo and instead take both the Class type index and the member pointer representation, but defaulting unknown seems error-prone.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeIndexIterator.cpp:1
+#include "llvm/DebugInfo/CodeView/TypeIndexIterator.h"
+
----------------
This should have one of those silly LLVM-style comment headers.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeIndexIterator.cpp:272
+  case TypeLeafKind::LF_FUNC_ID:
+    Refs.push_back({TiRefKind::IndexRef, 0, 1});
+    Refs.push_back({TiRefKind::TypeRef, 4, 1});
----------------
If we still had those Layout struct types, we could use `offsetof(FuncIdRecord::Layout, ParentScope)` etc here to avoid the hardcoded offsets. It would save work when we add new type records. Probably not a change for today, but maybe worth a FIXME comment?


https://reviews.llvm.org/D33564