[PATCH] D33009: [CodeView] Add an amortized O(1) random access type visitor

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks, one more round of comments and I think we're done. I'm just staring at data structure code trying to make it simple.



================
Comment at: llvm/lib/DebugInfo/CodeView/RandomAccessTypeVisitor.cpp:34
+
+  if (!Database.contains(TI)) {
+    PartialOffsetArray::Iterator Begin = PartialOffsets.end();
----------------
I think extracting this if block into a private method would improve readability. Using upper_bound is always tricky.


================
Comment at: llvm/lib/DebugInfo/CodeView/RandomAccessTypeVisitor.cpp:35
+  if (!Database.contains(TI)) {
+    PartialOffsetArray::Iterator Begin = PartialOffsets.end();
+    PartialOffsetArray::Iterator End = PartialOffsets.end();
----------------
Shouldn't this be `.begin()` not `.end()`? If it's empty, they'll be equivalent anyway.


================
Comment at: llvm/lib/DebugInfo/CodeView/RandomAccessTypeVisitor.cpp:48
+    TypeIndex TIB, TIE;
+    if (Begin != End) {
+      TIB = Begin->Type;
----------------
I think you can simplify this to three cases:
1. `PartialOffsets.empty()`: Scan all type indices. You can probably handle this step first and return early, so that Begin is always valid.
2. `std::next(Begin) == PartialOffsets.end()`: In this case, TIE is based on capacity like you have it.
3. Otherwise, TIE is `std::next(Begin)->Type`


================
Comment at: llvm/lib/DebugInfo/CodeView/RandomAccessTypeVisitor.cpp:50
+      TIB = Begin->Type;
+      End = Begin + 1;
+    } else {
----------------
Maybe `std::next`?


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeDatabase.cpp:159
 
-CVType &RandomAccessTypeDatabase::getTypeRecord(TypeIndex Index) {
-  assert(containsTypeIndex(Index));
-  return TypeDatabase::getTypeRecord(Index);
+  int Index = ValidRecords.find_last();
+  assert(Index != -1);
----------------
Isn't this a linear scan from the end of the bitvector? Does `Count` work instead?


https://reviews.llvm.org/D33009