labath marked 8 inline comments as done.
labath added inline comments.
================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:27
+struct Stream {
+ enum class StreamKind {
+ Hex,
----------------
clayborg wrote:
> Maybe "Encoding" might be a better name?
On it's own I agree Encoding would be better, but the term `Kind` is already
used by the EFLYAML implementation, so I think it's better to be consistent
with that.
================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:28
+ enum class StreamKind {
+ Hex,
+ SystemInfo,
----------------
clayborg wrote:
> Maybe "Binary" instead of "Hex"? Or maybe "RawBytes"?
Makes sense. In fact ELF already uses the name `RawContent` for the equivalent
functionality, so I went with that. I also changed `Text` to `TextContent`.
================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:50
+ yaml::BinaryRef Content;
+ yaml::Hex32 Size;
+
----------------
clayborg wrote:
> Is the "Size" necessary here? Does yaml::BinaryRef not contain a size?
This is another bit I stole from ELFYAML. The idea is that you can specify only
the size, in case you don't actually care about the content, or specify the
size+content if you only care about the initial few bytes of the section.
However, it looks like I didn't finish copying the idea, so I was missing the
bits that check that `Size >= Content.size()` and the bit which pads the size
when serializing. Now I fix that and add some additional tests.
================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:81
+/// /proc/<pid> file on linux).
+struct TextStream : public Stream {
+ BlockStringRef Text;
----------------
clayborg wrote:
> 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?
Unfortunately, I don't think there is a well-defined encoding here. I think the
files will be mostly ASCII, but for instance the encoding of the "filename"
part of /proc/PID/maps can really be anything. So, I think we should just keep
this bit deliberately vague.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59482/new/
https://reviews.llvm.org/D59482
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
