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