[Lldb-commits] [PATCH] D59482: [ObjectYAML] Add basic minidump generation support

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:27
+struct Stream {
+  enum class StreamKind {
+    Hex,
----------------
Maybe "Encoding" might be a better name?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:28
+  enum class StreamKind {
+    Hex,
+    SystemInfo,
----------------
Maybe "Binary" instead of "Hex"? Or maybe "RawBytes"?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:48
+/// fallback when no other stream kind is suitable.
+struct HexStream : public Stream {
+  yaml::BinaryRef Content;
----------------
Name "ByteStream" maybe? Does hex make sense?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:50
+  yaml::BinaryRef Content;
+  yaml::Hex32 Size;
+
----------------
Is the "Size" necessary here? Does yaml::BinaryRef not contain a size?


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:81
+/// /proc/<pid> file on linux).
+struct TextStream : public Stream {
+  BlockStringRef Text;
----------------
Maybe further specify the content like "UTF8Stream" or "ASCIIStream"? Not sure what the encoding is expected to be, but might be better to indicate the exact text encoding here?


================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:132
+std::unique_ptr<Stream> Stream::create(StreamType Type) {
+  StreamKind Kind = getKind(Type);
+  switch (Kind) {
----------------
This is where encoding might be a bit more clear?
```
Encoding E = getEncoding(Type);
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59482