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

[Pavel Labath via lldb-commits]

labath added a comment.

First round of comments from me :).



================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:248
+
+    if (lowest_addr.find(module_name) == lowest_addr.end()) {
+      lowest_addr[module_name] =
----------------
If you use the `emplace` function, and then check the returned value, you will avoid doing two lookups into the table.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:256
+
+  for (auto module : lowest_addr) {
+    filtered_modules.push_back(module.second.second);
----------------
`const auto &`, to avoid a copy. You can also call reserve() on the vector to avoid reallocations.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:387
+      info.SetReadable((entry->protect & PageNoAccess) == 0
+                           ? MemoryRegionInfo::eYes
+                           : MemoryRegionInfo::eNo);
----------------
these would probably fit on one line if you defined helper constants "yes" and "no" like you did in the tests.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:79
+  std::vector<const MinidumpModule *>
+  FilterModuleList(llvm::ArrayRef<MinidumpModule> &modules);
+
----------------
This would be easier to use if was declared like this:
```
... GetFilteredModuleList();
```
and it called GetModuleList under the hood. (Unless you actually anticipate needing to filter random module lists, in which case it should probably be static as it does not touch the local vars). And in any case, it should not take a reference to the ArrayRef.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:85
+
+  size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size, Error &error);
+
----------------
We use `Do` prefix is used for internal helper functions, when you cannot think of a better name.  This functions is not that, so it should be just `ReadMemory`. That said, I don't think this function should be here at all.
An interface more consistent with the rest of the class would be:
```
llvm::ArrayRef<uint8_t> GetMemory(lldb::addr_t addr, size_t size);
```
It involves no copying (but the caller can still easily memcpy() it if he really needs to. As for the corner cases (callee asks for a region which is only partially available, you can define whichever behaviour makes more sense for your use case. If after this, you find that the function is too small to be useful, you can keep it as a private function inside the register context class.



================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:87
+
+  Error GetMemoryRegionInfo(lldb::addr_t load_addr,
+                            lldb_private::MemoryRegionInfo &info);
----------------
```
llvm::Optional<MemoryRegionInfo> GetMemoryRegionInfo(lldb::addr_t)
```
seems more consistent.


================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:144
+      parser->FilterModuleList(modules);
+  EXPECT_GT(modules.size(), filtered_modules.size());
+  bool found = false;
----------------
Why not expect the exact numbers here? They're not going to change between runs.


https://reviews.llvm.org/D25569