[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

Code basically looks fine, but somebody else should confirm that the format is correct.



================
Comment at: lib/Object/Minidump.cpp:55
+  if (!OptionalStream)
+    return createError("No such stream", object_error::invalid_section_index);
+  auto ExpectedSize =
----------------
I wonder whether it would be worth a new class of errors for minidump files? After all, invalid_section_index feels a bit forced for a format without sections!


================
Comment at: unittests/Object/MinidumpTest.cpp:317
+      1, 0, 0, 0,                           // NumberOfStreams,
+      0x20, 0, 0, 0,                        // StreamDirectoryRVA
+      0, 1, 2, 3, 4, 5, 6, 7,               // Checksum, TimeDateStamp
----------------
It's a bit jarring seeing both hex and decimal numbers in here at the same time. Is there a particular reason you've used hex here, but decimal for e.g. 116 below?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60121