zturner added a comment. In D58976#1418621 <https://reviews.llvm.org/D58976#1418621>, @clayborg wrote:
> Strong ownership is needed for this class IMHO because it hands out pointers > to native data I disagree here, see my previous comment. Binaries grow large very quickly, and if we always copy data around when we want to hand out some internal pointer, memory usage would explode. Luckily, the scenario that attempts to prevent is very rare. Specifically, it only addresses the scenario where you open a file, parse a bunch of data, close the file, then still expect to be able to do something with the file's internal data structures. I haven't ever seen this be a problem in practice, as the "natural" order is to open a file, process it, then close the file. And in that case this is fine. I don't mind having it be documented in the class header, but given that the pattern is fairly pervasive in LLVM (e.g. all of lib/Object works this way, among other things), I'm also fine with just letting it be implicitly understood. ================ Comment at: include/lldb/Formats/MinidumpParser.h:105 private: - lldb::DataBufferSP m_data_sp; + llvm::ArrayRef<uint8_t> m_data; + const MinidumpHeader *m_header; ---------------- clayborg wrote: > I worry about this going stale when the owner of the data lets it go and we > crash now that we don't have strong ownership. If this is common in LLVM, > then we need to document this in the header file. This is a fairly pervasive, as well as being an important optimization. We don't want to be copying data around from binary files because the amount of data that ends up being copied could quickly spiral out of control since binaries get quite large. The semantics are that the data is valid as long as the backing file remains opened. Anyone using the class needs to be aware of this, and if they want a lifetime that is not tied to the life of the backing file (which is a fairly uncommon scenario), they need to explicitly copy any data they need to outlive the file. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58976/new/ https://reviews.llvm.org/D58976 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
