[Lldb-commits] [PATCH] D61423: MinidumpYAML: add support for the ThreadList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:57
+/// instantiations can be used to represent the ModuleList stream and other
+/// streams with similar structure.
+template <typename EntryT, Stream::StreamKind KindV, minidump::StreamType TypeV>
----------------
with -> with a


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:58
+/// streams with similar structure.
+template <typename EntryT, Stream::StreamKind KindV, minidump::StreamType TypeV>
+struct ListStream : public Stream {
----------------
KindV and TypeV aren't clear names to me. What does the V stand for?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:64
 
-  static bool classof(const Stream *S) {
-    return S->Kind == StreamKind::ModuleList;
-  }
+  ListStream(std::vector<entry_type> Entries = {})
+      : Stream(KindV, TypeV), Entries(std::move(Entries)) {}
----------------
`explicit`?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70-73
+/// A structure containing all data belonging to a single minidump module. On
+/// disk, these are placed at various places in the minidump file and
+/// cross-referenced via their offsets, but for ease of use, we group them
+/// together in the logical memory view.
----------------
I'm not sure how much sense it makes to go into the detail of the minidump file format versus the memory view here. I also am not convinced by the repetition of this in the comments below.


================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:481
+
+template <typename EntryT, Stream::StreamKind KindV, StreamType TypeV>
+static size_t
----------------
Same comment as above re. names.


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:47-49
+      - Thread Id:       0x5C5D5E5F
+        Priority Class:  0x60616263
+        Environment Block: 0x6465666768696A6B
----------------
It would be nice if these were padded so that they all line up. Ditto in the Stack block below.


================
Comment at: test/tools/obj2yaml/basic-minidump.yaml:51
+        Stack:           
+          Start of Memory Range: 0x6C6D6E6F70717273
+          Content:         7475767778797A7B
----------------
I don't have a concrete suggestion, but it might be nice to have a shorter field name than "Start of Memory Range", but that's less of a concern if that's the actual minidump field name.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61423