labath marked 11 inline comments as done.
labath added inline comments.
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:181
+ FeaturesRef.writeAsBinary(FeaturesStream);
+ std::fill(Features.begin(), Features.end(), 0);
+ std::memcpy(Features.begin(), FeaturesStorage.begin(),
----------------
jhenderson wrote:
> Is this definitely necessary? What's the type of Info.ProcessorFeatures?
> Similar comments for Vendor ID below.
No, it's not necessary, it's just that I was too lazy to implement proper error
checking for this (and this field isn't that important -- I don't believe we
actually use it for anything right now, but I have to print it in some way).
I've now implemented proper checking for this and the VendorID fields
(including tests).
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:306
+Error MinidumpYAML::writeAsBinary(StringRef Yaml, raw_ostream &OS) {
+ yaml::Input yin(Yaml);
+ Object Obj;
----------------
jhenderson wrote:
> I don't think `yin` matches our (current) variable naming guidelines. It
> should be `Yin` at least, though it wasn't immediately clear what was meant
> even then at first to me, so perhaps a different name might be better (simply
> `Input` would work for me).
There's definitely plenty of places that use `yin`, with the yaml library
itself being the most prominent user. I think you could argue that it fits the
naming guidelines due to the "emulates c++ stl interface" (i.e `cin`)
exception, but I don't think the name really matters that much here, so I've
just renamed it to `Input`.
================
Comment at: tools/yaml2obj/yaml2obj.cpp:61
+ if (Doc.Minidump) {
+ MinidumpYAML::writeAsBinary(*Doc.Minidump, Out);
+ return 0;
----------------
jhenderson wrote:
> I feel like it would make more sense for this to match the other versions
> above, i.e. I think this should be at least `return yaml2minidump(...);`
Yeah, I had the same feeling, but then I also didn't like creating a
`yaml2minidump.cpp` just to put a two-line function in it (and putting this
function into the ObjectYAML library also didn't feel right).
The root cause here is that I've chose to fully library-ize the minidump code,
which doesn't fit in with how the other object files work (though there is a
kind of a precedent for this in DWARFYAML). My reason for doing that was that I
think it would be very useful to have this functionality accessible to lldb
unit tests (and I've also gotten interest from a downstream user who wanted to
use this functionality to synthesize minidumps in their unit tests).
Long story short, I've now put the created yaml2minidump.cpp and put the
yaml2minidump in it.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59482/new/
https://reviews.llvm.org/D59482
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
