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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits