[Lldb-commits] [PATCH] D25569: Minidump plugin: functions parsing memory structures and filtering module list

[Zachary Turner via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:242
+
+  for (const auto &module : modules) {
+    name = GetMinidumpString(module.module_name_rva);
----------------
I don't know how big the minidumps you're working with are or if performance is a concern, but `std::map<>` is more or less a piece of junk.  The only time it should really be used is if you need a deterministic iteration order.  `std::unordered_map<>` is slightly better.  Better still is `llvm::StringMap<>`


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:281
+
+  if (data.size() == 0 && data64.size() == 0)
+    return llvm::None;
----------------
`data.empty()`?


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:284
+
+  if (data.size() > 0) {
+    llvm::ArrayRef<MinidumpMemoryDescriptor> memory_list =
----------------
`!data.empty()`?


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:291
+
+    for (auto memory_desc : memory_list) {
+      const MinidumpLocationDescriptor &loc_desc = memory_desc.memory;
----------------
`auto &` to avoid a copy.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:320
+
+    for (auto memory_desc64 : memory64_list) {
+      const lldb::addr_t range_start = memory_desc64.start_of_memory_range;
----------------
`auto &`


================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:222
+
+  if (header->size_of_header > sizeof(MinidumpMemoryInfoListHeader)) {
+    data = data.drop_front(header->size_of_header -
----------------
I don't think you need the conditional here do you?  Just `data = data.drop_front(sizeof_MinidumpMemoryInfoListHeader)`.


================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:268
+  MemFree = 0x10000,
+  MemReserve = 0x2000,
+};
----------------
Can you use `llvm/ADT/BitmaskEnum.h` here to give this enum bitwise operators?


================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:274
+  MemMapped = 0x40000,
+  MemPrivate = 0x20000,
+};
----------------
Same


================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:294
+  PageExecutable = PageExecute | PageExecuteRead | PageExecuteReadWrite |
+                   PageExecuteWriteCopy,
+};
----------------
Same


https://reviews.llvm.org/D25569