[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I would still vote to check Buffer for NULL. GetByteSize() and GetBytes() are usually accessed one time so there won't be a performance issue. If anyone wanted to actually use a DataBufferLLVM as a member variable, they would need to use a std::unique_ptr to one with the current designed because it can't be default constructed.



================
Comment at: lldb/source/Core/DataBufferLLVM.cpp:20-21
+    : Buffer(std::move(Buffer)) {
+  assert(Buffer != nullptr &&
+         "Cannot construct a DataBufferLLVM with a null buffer");
+}
----------------
This doesn't stop things from crashing if for some reason someone does do this and asserts are off. The DataBuffer classes are there for keeping the data alive. The main class that uses the DataBuffer classes is DataExtractor and that grabs the data pointer on time and stores it into the DataExtractor class. So I vote for letting this be empty and checking the unique_ptr as GetBytes is usually accessed one time.


================
Comment at: lldb/source/Core/DataBufferLLVM.cpp:52
+lldb::offset_t DataBufferLLVM::GetByteSize() const {
+  return Buffer->getBufferSize();
+}
----------------
I would still vote to check Buffer for NULL. As I said above, GetByteSize() and GetBytes() are usually accessed one time.


================
Comment at: lldb/source/Core/DataBufferLLVM.cpp:56
+const uint8_t *DataBufferLLVM::GetBuffer() const {
+  return reinterpret_cast<const uint8_t *>(Buffer->getBufferStart());
+}
----------------
I would still vote to check Buffer for NULL. As I said above, GetByteSize() and GetBytes() are usually accessed one time.


https://reviews.llvm.org/D30054