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

Reid Kleckner via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 9 13:46:14 PDT 2017

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')`.


More information about the llvm-commits mailing list