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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
