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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:49
+  size_t size = 0;
+  size += sizeof(m_ht->base);
+  size += sizeof(m_ht->bits);
----------------
Shouldn't we check that `m_ht` is actually managing an object before attempting to access it's value?


================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:50
+  size += sizeof(m_ht->base);
+  size += sizeof(m_ht->bits);
+
----------------
These `sizeof` calls feel like the should just be consolidated into the initialization of `size`.


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:698
+  const char *item_name = name.GetCString();
+  uint32_t idx = ExtractIndexFromString(item_name);
+  if (idx < UINT32_MAX && idx >= CalculateNumChildren())
----------------
`const`


================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:738
+
+  uint32_t num_children = CalculateNumChildren();
+
----------------
`const`


================
Comment at: lldb/source/Plugins/Language/ObjC/NSSet.cpp:601
+
+  uint32_t num_children = CalculateNumChildren();
+
----------------
`const` if a value is not supposed to change make it `const` always. This prevents bugs where a "const" is modified by mistake.


================
Comment at: lldb/source/Plugins/Language/ObjC/NSSet.cpp:647
+    case 4:
+      *((uint32_t *)buffer.GetBytes()) = (uint32_t)set_item.item_ptr;
+      break;
----------------
`reinterpret_cast` and `static_cast` respectively. Same below. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78396