[Lldb-commits] [PATCH] D61064: Object/Minidump: Add support for the ThreadList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Aside from a couple of nits, LGTM.



================
Comment at: lib/Object/Minidump.cpp:69
   size_t ListOffset = 4;
-  // Some producers insert additional padding bytes to align the module list to
-  // 8-byte boundary. Check for that by comparing the module list size with the
-  // overall stream size.
-  if (ListOffset + sizeof(Module) * ListSize < OptionalStream->size())
+  // Some producers insert additional padding bytes to align the list to 8-byte
+  // boundary. Check for that by comparing the list size with the overall stream
----------------
Nit (was there before): to 8-byte -> to an 8-byte


================
Comment at: unittests/Object/MinidumpTest.cpp:446
+
+  for (const std::vector<uint8_t> &Data : {OneThread, PaddedThread}) {
+    auto ExpectedFile = create(Data);
----------------
I missed this in the other tests, but this could be an `ArrayRef<uint8_t>` instead of a `const std::vector<uint8_t> &`. Feel free to leave it as is or to update it in an NFC throughout this test afterwards, if you would prefer to leave it to another patch.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61064/new/

https://reviews.llvm.org/D61064