[PATCH] D36535: [LLD/PDB] Output actual records to the globals stream

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/DebugInfo/CodeView/RecordName.cpp:257-258
+    // See ProcSym
+    StringData = Sym.content().drop_front(35);
+    break;
+  case SymbolKind::S_THUNK32:
----------------
LLVM frequently factors large switches like this into separate functions. In this case, you can return the offset or some sentinel value like 0 or -1. This saves duplication of `Sym.content().drop_front(Offset)` and `break` becomes `return 35`.


================
Comment at: llvm/lib/DebugInfo/CodeView/RecordName.cpp:325-327
+    // S_CONSTANT is preceded by an APSInt, which has a variable length.  So we
+    // have to do a full deserialization.
+    BinaryStreamReader Reader(Sym.content(), llvm::support::little);
----------------
Ouch, I was hoping all names were at constant offsets. We can put this in a separate `if` to implement the previously suggested refactoring.


================
Comment at: llvm/lib/DebugInfo/CodeView/RecordName.cpp:342-343
+  // everything up to but not including the first null terminator.
+  StringData = StringData.take_until([](uint8_t N) { return N == 0; });
+  return toStringRef(StringData);
+}
----------------
IMO it would be nicer to avoid the use of the lambda. We have alternatives like `strnlen` and `.split('\0')`.


https://reviews.llvm.org/D36535