[Lldb-commits] [PATCH] D59291: [Object] Add basic minidump support

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

I haven't really looked at the behaviour to make sure it makes sense, but I've made a number of comments on the comments and one or two other things.



================
Comment at: include/llvm/Object/Minidump.h:25
+  /// Construct a new MinidumpFile object from the given memory buffer. Returns
+  /// an error if this file cannot be identified as a minidump file, or if it's
+  /// contents are badly corrupted (i.e., we cannot read the stream directory).
----------------
it's -> its


================
Comment at: include/llvm/Object/Minidump.h:26
+  /// an error if this file cannot be identified as a minidump file, or if it's
+  /// contents are badly corrupted (i.e., we cannot read the stream directory).
+  static Expected<std::unique_ptr<MinidumpFile>> create(MemoryBufferRef Source);
----------------
Nit, you don't need the ',' after 'i.e.'.


================
Comment at: include/llvm/Object/Minidump.h:42
+
+  /// Returns raw the contents of the stream of the given type, or None if the
+  /// file does not contain a stream of this type.
----------------
raw the -> the raw


================
Comment at: include/llvm/Object/Minidump.h:61
+  static Error createEOFError() {
+    return createError("Unexpected EOF.", object_error::unexpected_eof);
+  }
----------------
I've seen in some places that the error handling automatically appends a full stop, so this would result in two full stops, which is probably not ideal. Also, having a full stop prevents a consumer producing the error message in the middle of a quote or similar, e.g. "the library produced the following error 'Unexpected EOF' and will terminate" or whatever.

Same comment applies in a few other places.


================
Comment at: include/llvm/Object/Minidump.h:68
+
+  /// Return the slice of the given data array as an array of objects of given
+  /// type. The function checks that the input array is large enough to contain
----------------
of given -> of a given


================
Comment at: include/llvm/Object/Minidump.h:77
+               ArrayRef<minidump::Directory> Streams,
+               std::unordered_map<minidump::StreamType, std::size_t> StreamMap)
+      : Binary(ID_Minidump, Source), Header(Header), Streams(Streams),
----------------
Are you deliberately making a copy of StreamMap? I would normally expect this to be passed by some form of reference.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59291