[Lldb-commits] [PATCH] D55472: Speadup memory regions handling and cleanup related code

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Woohoo for speedups.

However, I noticed that you are modifying the signatures of SB functions, which I don't believe will be acceptable for lldb.



================
Comment at: include/lldb/API/SBMemoryRegionInfo.h:105
 
-  SBMemoryRegionInfo(const lldb_private::MemoryRegionInfo *lldb_object_ptr);
+  SBMemoryRegionInfo(lldb::MemoryRegionInfoUP &&lldb_object_up);
+
----------------
Generally, I think it's better to use values here instead of rvalue references, as they make clearer interfaces (a function which receives the a unique_ptr by value has to consume it whether it likes to or not, but in case of rvalue references, the object will survive the function call unless the function explicitly does something to it).

I don't believe this should matter for performance, as that's just copying the pointer value around.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:404
 
+namespace {
+  constexpr uint32_t StateMemFree =
----------------
Namespaces are not indented in llvm style.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:408
+
+  MemoryRegionInfo MemoryInfoToRegionInfo(const MinidumpMemoryInfo &entry) {
+  const auto yes = MemoryRegionInfo::eYes;
----------------
llvm prefers static functions instead of ones in anonymous namespaces


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55472