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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

I am going to stop making comments as I have been working on a similar patch that does more than this one. I will post it today and we can decide which approach we want to use.



================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:29
 
-  uint32_t GetSize() const;
+  size_t GetSize() const;
 
----------------
Revert. You can add functions, but never change them. Our API is stable. 


================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:31
 
-  bool GetMemoryRegionAtIndex(uint32_t idx, SBMemoryRegionInfo &region_info);
+  void Reserve(size_t capacity);
 
----------------
This you can add, but std::vector implementations are quite efficient and I doubt this is really needed?


================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:33
 
-  void Append(lldb::SBMemoryRegionInfo &region);
+  bool GetMemoryRegionAtIndex(size_t idx, SBMemoryRegionInfo &region_info);
 
----------------
Revert to maintain API


================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:35
 
-  void Append(lldb::SBMemoryRegionInfoList &region_list);
+  void Append(const lldb::SBMemoryRegionInfo &region);
+
----------------
"const" means nothing to our objects as they have shared pointers or unique pointers which never change. 


================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:37
+
+  void Append(const lldb::SBMemoryRegionInfoList &region_list);
 
----------------
ditto, and revert for API


================
Comment at: include/lldb/Target/Process.h:2084
   virtual Status
-  GetMemoryRegions(std::vector<lldb::MemoryRegionInfoSP> &region_list);
+  GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> &region_list);
 
----------------
This was a a vector of shared pointers, seemingly for no good reason prior to this change, I would change this to be a vector of MemoryRegionInfo instances? 


================
Comment at: source/API/SBMemoryRegionInfo.cpp:23-24
 
-SBMemoryRegionInfo::SBMemoryRegionInfo(const MemoryRegionInfo *lldb_object_ptr)
-    : m_opaque_ap(new MemoryRegionInfo()) {
-  if (lldb_object_ptr)
-    ref() = *lldb_object_ptr;
-}
+SBMemoryRegionInfo::SBMemoryRegionInfo(MemoryRegionInfoUP &&lldb_object_up)
+    : m_opaque_ap(std::move(lldb_object_up)) {}
 
----------------
clayborg wrote:
> Don't remove... API
Fine to add to API, but really no one outside LLDB will be able to do anything with this function (nor the one on the left) since MemoryRegionInfo is opaque as far as the public clients are concerned. It is there for internal LLDB convenience. So do we really need this? We can add a method that copies from an instance if needed. MemoryRegionInfo structs are very simple to copy. No need for heroics here.


================
Comment at: source/API/SBMemoryRegionInfo.cpp:23-27
-SBMemoryRegionInfo::SBMemoryRegionInfo(const MemoryRegionInfo *lldb_object_ptr)
-    : m_opaque_ap(new MemoryRegionInfo()) {
-  if (lldb_object_ptr)
-    ref() = *lldb_object_ptr;
-}
----------------
Don't remove... API


================
Comment at: source/API/SBMemoryRegionInfo.cpp:39-44
+SBMemoryRegionInfo &SBMemoryRegionInfo::
+operator=(MemoryRegionInfoUP &&lldb_object_up) {
+  m_opaque_ap = std::move(lldb_object_up);
+  return *this;
+}
+
----------------
Do this by instance instead of a move assignment operator with a unique ptr?


================
Comment at: source/API/SBMemoryRegionInfoList.cpp:81
 
-uint32_t SBMemoryRegionInfoList::GetSize() const {
+size_t SBMemoryRegionInfoList::GetSize() const {
   return m_opaque_ap->GetSize();
----------------
revert


================
Comment at: source/API/SBMemoryRegionInfoList.cpp:85-88
+void SBMemoryRegionInfoList::Reserve(size_t capacity) {
+  m_opaque_ap->Reserve(capacity);
+}
+
----------------
Who needs this externally? I would rather not add this to the public API? 


================
Comment at: source/API/SBMemoryRegionInfoList.cpp:90
 bool SBMemoryRegionInfoList::GetMemoryRegionAtIndex(
-    uint32_t idx, SBMemoryRegionInfo &region_info) {
+    size_t idx, SBMemoryRegionInfo &region_info) {
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
----------------
revert


================
Comment at: source/API/SBMemoryRegionInfoList.cpp:110
 
-void SBMemoryRegionInfoList::Append(SBMemoryRegionInfo &sb_region) {
+void SBMemoryRegionInfoList::Append(const SBMemoryRegionInfo &sb_region) {
   m_opaque_ap->Append(sb_region);
----------------
remove const


================
Comment at: source/API/SBMemoryRegionInfoList.cpp:115
+void
+SBMemoryRegionInfoList::Append(const SBMemoryRegionInfoList &sb_region_list) {
   m_opaque_ap->Append(*sb_region_list);
----------------
revert


================
Comment at: source/API/SBProcess.cpp:1367
+
+      MemoryRegionInfoUP region_info_up(new lldb_private::MemoryRegionInfo());
       sb_error.ref() =
----------------
Remove. "sb_region_info" already has an instance we can use.


================
Comment at: source/API/SBProcess.cpp:1369
       sb_error.ref() =
-          process_sp->GetMemoryRegionInfo(load_addr, *region_info_sp);
+          process_sp->GetMemoryRegionInfo(load_addr, *region_info_up);
       if (sb_error.Success()) {
----------------
Change Process::GetMemoryRegionInfo() to use an "MemoryRegionInfo &region" instead of shared pointer or unique pointer?


================
Comment at: source/API/SBProcess.cpp:1371
       if (sb_error.Success()) {
-        sb_region_info.ref() = *region_info_sp;
+        sb_region_info.m_opaque_ap = std::move(region_info_up);
       }
----------------
If above changes are made this is not needed.


================
Comment at: source/API/SBProcess.cpp:1399
+
+    std::vector<MemoryRegionInfoUP> region_list;
+    if (process_sp->GetMemoryRegions(region_list).Success()) {
----------------
Just use the std::vector<MemoryRegionInfo> from MemoryRegionInfoListImpl inside the "sb_region_list" already?


================
Comment at: source/API/SBProcess.cpp:1400
+    std::vector<MemoryRegionInfoUP> region_list;
+    if (process_sp->GetMemoryRegions(region_list).Success()) {
+      sb_region_list.Reserve(sb_region_list.GetSize() + region_list.size());
----------------
Change Process:: GetMemoryRegions() to take a "std::vector<MemoryRegionInfo>&region_list"?


================
Comment at: source/API/SBProcess.cpp:1401
+    if (process_sp->GetMemoryRegions(region_list).Success()) {
+      sb_region_list.Reserve(sb_region_list.GetSize() + region_list.size());
+      for (auto& region : region_list)
----------------
This doesn't need to be done if the above code is changed


================
Comment at: source/API/SBProcess.cpp:1402-1403
+      sb_region_list.Reserve(sb_region_list.GetSize() + region_list.size());
+      for (auto& region : region_list)
+        sb_region_list.Append(SBMemoryRegionInfo(std::move(region)));
     }
----------------
This doesn't need to be done if the above code is changed to just pass the vector of regions from "sb_region_list"


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:91
 
+  Status GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> &region_list);
+
----------------
switch to all internal GetMemoryRegions calls to just use "std::vector<lldb::MemoryRegionInfo>"?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55472