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

[James Henderson via Phabricator via lldb-commits]

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