[Lldb-commits] [PATCH] D23545: Minidump parsing
Dimitar Vlahovski via lldb-commits
lldb-commits at lists.llvm.org
Thu Aug 18 11:03:25 PDT 2016
dvlahovski added inline comments.
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+llvm::Optional<const MinidumpHeader *>
> 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);
> 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)
More information about the lldb-commits