the stream format is exactly the same as for ThreadList and ModuleList
streams, only the entry types are slightly different, so the changes in
this patch are just straight-forward applications of established
patterns.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
include/llvm/Object/Minidump.h | ||
---|---|---|
85–86 ↗ | (On Diff #199384) | These two lines talk about threads. Is that a copy/paste error? |
include/llvm/ObjectYAML/MinidumpYAML.h | ||
92 ↗ | (On Diff #199384) | Would it make more sense to call this ParsedMermoryList, to match the StreamType? |
test/tools/obj2yaml/basic-minidump.yaml | ||
54–57 ↗ | (On Diff #199384) | 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. |
unittests/Object/MinidumpTest.cpp | ||
483 ↗ | (On Diff #199384) | thread? Copy/paste error? |
Address review comments.
include/llvm/Object/Minidump.h | ||
---|---|---|
85–86 ↗ | (On Diff #199384) | Yep, sorry. This is the downside of "straight-forward applications of established |
include/llvm/ObjectYAML/MinidumpYAML.h | ||
92 ↗ | (On Diff #199384) | Not really, because this represents only one entry in the MemoryList stream, and not list as a whole. However, we could call it ParsedMemoryDescriptor, as that's the type of the list entries. |
test/tools/obj2yaml/basic-minidump.yaml | ||
54–57 ↗ | (On Diff #199384) | Done. The different quoting of is actually a relict of how obj2yaml prints BinaryRef values (they omit quotes if the data happens to be contain hex (A-F) characters). Do you think it would be worth making this output more consistent too? |
LGTM, though I haven't bothered to inspect the detail of the minidump format. I'll leave it up to you whether you think it's worth getting a separate pair of eyes.
test/tools/obj2yaml/basic-minidump.yaml | ||
---|---|---|
54–57 ↗ | (On Diff #199384) | Probably, but that sounds like an unrelated changes. |
Thanks. This is very straight-forward, so I don't think another set of eyes is needed (though I expect Greg is keeping at least one eye on this).