jhenderson added a comment.
I've not reviewed the unit test yet, but the bulk of the body looks fine, apart
from some minor details.
================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:27
+struct Stream {
+ enum StreamKind {
+ HexKind,
----------------
I think it is worth making this an `enum class`.
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:55
+ Callback(OS);
+ assert(OS.tell() == BeginOffset + NextOffset);
+ (void)BeginOffset;
----------------
Perhaps worth a quick message:
`assert(OS.tell() == BeginOffset + NextOffset && "some message here");`
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:70
+template <typename MapType, typename EndianType>
+static inline void mapAs(yaml::IO &IO, const char *Key, EndianType &Val) {
+ MapType Mapped = static_cast<typename EndianType::value_type>(Val);
----------------
Maybe worth calling this `mapRequiredAs`, to line up with the `mapRequired`
name.
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:96
+template <typename EndianType>
+static inline void mapHex(yaml::IO &IO, const char *Key, EndianType &Val) {
+ mapAs<typename HexType<EndianType>::type>(IO, Key, Val);
----------------
Similar to above, perhaps `mapRequiredHex`?
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:100
+
+/// Perform an optionam yaml-mapping of an endian-aware type as an
+/// appropriately-sized hex value.
----------------
`optionam`?
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:181
+ FeaturesRef.writeAsBinary(FeaturesStream);
+ std::fill(Features.begin(), Features.end(), 0);
+ std::memcpy(Features.begin(), FeaturesStorage.begin(),
----------------
Is this definitely necessary? What's the type of Info.ProcessorFeatures?
Similar comments for Vendor ID below.
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:183
+ std::memcpy(Features.begin(), FeaturesStorage.begin(),
+ std::min(FeaturesStorage.size(), Features.size()));
+}
----------------
If there is too much data to fit, should this emit an error, rather than
silently truncate? Same below with Vendor ID.
================
Comment at: lib/ObjectYAML/MinidumpYAML.cpp:306
+Error MinidumpYAML::writeAsBinary(StringRef Yaml, raw_ostream &OS) {
+ yaml::Input yin(Yaml);
+ Object Obj;
----------------
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).
================
Comment at: tools/yaml2obj/yaml2obj.cpp:61
+ if (Doc.Minidump) {
+ MinidumpYAML::writeAsBinary(*Doc.Minidump, Out);
+ return 0;
----------------
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(...);`
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
