[Lldb-commits] [PATCH] D78396: [lldb/Dataformatter] Add support for CoreFoundation Dictionaries and Sets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't have a vested interested here (the main reason I looked at this was the template discussion), but I have made a cursory pass over the patch. I didn't check the high level functionality, but it seems like the implementation details can be improved upon.



================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:45
+  addr_t addr = m_address.GetLoadAddress(target);
+  size_t size = sizeof(m_ht->base) + sizeof(m_ht->bits);
+
----------------
I am not sure, but I think this still may technically be undefined behavior if `m_ht` is null even though the application of `sizeof` means the dereference will not normally take place.


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:47-62
+  DataBufferHeap buffer(size, 0);
+  m_exe_ctx_ref.GetProcessSP()->ReadMemory(addr, buffer.GetBytes(), size,
+                                           error);
+  if (error.Fail())
+    return false;
+
+  DataExtractor data_extractor = DataExtractor(
----------------
Why don't you just pass `m_ht.get()` to `ReadMemory` ?


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:71-82
+  m_exe_ctx_ref.GetProcessSP()->ReadMemory(ptr_offset, buffer.GetBytes(), size,
+                                           error);
+
+  if (error.Fail()) {
+    m_ht = nullptr;
+    return false;
+  }
----------------
`ReadMemory(ptr_offset, m_ht->pointers, ...)` ?


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:692-693
+
+lldb_private::formatters::NSCFDictionarySyntheticFrontEnd::
+    ~NSCFDictionarySyntheticFrontEnd() {}
+
----------------
`= default`? Maybe even implicitly default by not providing a destructor?


================
Comment at: lldb/source/Plugins/Language/ObjC/NSSet.cpp:666-667
+
+    DataExtractor data(buffer.GetBytes(), buffer.GetByteSize(), m_order,
+                       m_ptr_size);
+
----------------
You've cargo-culted this code, but you've changed `DataBufferSP` to a plain `DataBuffer`. If the created valueobject continues to reference this data (I haven't checked, but I think it does), this will be a dangling reference.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78396/new/

https://reviews.llvm.org/D78396