[Lldb-commits] [PATCH] D23545: Minidump parsing

[Dimitar Vlahovski via lldb-commits]

dvlahovski added inline comments.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+
+llvm::Optional<const MinidumpHeader *>
+MinidumpHeader::Parse(llvm::ArrayRef<uint8_t> &data)
----------------
zturner wrote:
> I think these can all just return the pointer instead of `llvm::Optional<>`.  return `nullptr` to indicate failure.  Optionally, make the signature be something like `Error MinidumpHeader::Parse(llvm::ArrayRef<uint8_t> &data, const MinidumpHeader *&Header)` which would allow you to propagate the error up (if you wanted to).
> 
> At the very least though, there's no point using `llvm::Optional<>` if `nullptr` can be used to indicate failure.
Yes, fair point. Now that I'm returning pointers, `nullptr` is better for indicating failure.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:30
@@ +29,3 @@
+    const MinidumpHeaderConstants version =
+        static_cast<const MinidumpHeaderConstants>(static_cast<const uint32_t>(header->version) & 0x0000ffff);
+
----------------
zturner wrote:
> Where does the `0xFFFF` come from?
There is a comment in the header, but probably it should be here. The higher 16 bit of the version field are implementation specific.
The lower 16 bits are the version number (which is always the same constant number)


https://reviews.llvm.org/D23545