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