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

[Zachary Turner via lldb-commits]

zturner added a comment.

Were you still going to change all the `Optionals` to raw pointers (or even better, `llvm::Errors`)


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:21
@@ +20,3 @@
+MinidumpParser::MinidumpParser(const lldb::DataBufferSP &data_buf_sp)
+    : m_data_sp(data_buf_sp), m_header(nullptr), m_valid_data(true)
+{
----------------
I'm not crazy about the idea of allowing the constructor to fail and then checking `IsValidData()` after you construct it.  One way LLVM solves this is by having a static function that returns `Expected<T>`.  You would use it like this:

```
Expected<MinidumpParser> MinidumpParser::create(const lldb::DataBufferSP &data_buf_sp) {
  if (data_buf_sp->GetByteSize() < sizeof(MinidumpHeader))
    return make_error<StringError>("Insufficient buffer!");

  MinidumpHeader *header = nullptr;
  if (auto EC = MinidumpHeader::Parse(header_data, header))
    return std::move(EC);

   ...

   return MinidumpParser(header, directory_map);
}

auto Parser = MinidumpParser::create(buffer);
if (!Parser) {
  // Handle the error
}
  
```

This way it's impossible to create a MinidumpParser that is invalid.

Up to you whether you want to do this, but I think it is a very good (and safe) pattern that I'd love to see LLDB start adopting more often.


https://reviews.llvm.org/D23545