[PATCH] D59775: Minidump: Add support for reading/writing strings

[Pavel Labath via Phabricator via llvm-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70
   minidump::SystemInfo Info;
+  std::string CSDVersion;
 
----------------
jhenderson wrote:
> Don't know about lifetime here, but could this be a `StringRef`?
A StringRef would work when constructing this object from YAML (as there the yaml file can provide backing storage). However, when constructing this from a Minidump binary, that would not work, as the data is stored there in the utf16 form (so not the kind of "string" we usually work with).

It would probably be possible to pull of some kind of a trick like `yaml::BinaryRef` does, where this would be a special object, which could be backed either by a real (utf8) string, or by the minidump utf16 thingy. However, I don't think that would be worth the trouble, as these strings are fairly small. (Also, in order to validate the utf16 data (so I don't end up creating an invalid object and crash during serialization), I'd have to essentially decode the data into a std::string anyway and then throw the result away.)


================
Comment at: unittests/Object/MinidumpTest.cpp:270
+      0,                                    // Mis-align next string
+      2, 0, 0, 0, 'a', 0,                   // String6 - "a"
+
----------------
jhenderson wrote:
> What about a case where the size field cannot fit in the remaining data?
Ah yes, good catch. I'll add that.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59775/new/

https://reviews.llvm.org/D59775