[Lldb-commits] [PATCH] D61885: Minidump: Add support for the MemoryList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/Object/Minidump.h:85-86
+  /// error is returned if the file does not contain this stream, or if the
+  /// stream is not large enough to contain the number of threads declared in
+  /// the stream header. The consistency of the Thread entries themselves is not
+  /// checked in any way.
----------------
These two lines talk about threads. Is that a copy/paste error?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:92
+/// A structure containing all data describing a single memory region.
+struct ParsedMemoryRange {
+  static constexpr Stream::StreamKind Kind = Stream::StreamKind::MemoryList;
----------------
Would it make more sense to call this ParsedMermoryList, to match the StreamType?


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:54-57
+  - Type:            MemoryList
+    Memory Ranges:   
+      - Start of Memory Range: 0x7C7D7E7F80818283
+        Content:         '8485868788'
----------------
I'd probably find this neater if the Indentation of values for each entry were more consistent, but I'm not too fussed.

Also, in the ThreadList above, the Content is not quoted, but here it is. Please standardise it on one or the other.


================
Comment at: unittests/Object/MinidumpTest.cpp:483
+  };
+  // Same as before, but with a padded thread list.
+  std::vector<uint8_t> PaddedRange{
----------------
thread? Copy/paste error?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61885