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

[Tatyana Krasnukha via Phabricator via lldb-commits]

tatyana-krasnukha added a comment.

Thanks to all for comments!

As I wrote in inline comments, 'non-const' -> 'const' changes don't seem to break anything, I'd like to leave it here. Correct me, if I'm wrong.

The second question is whether it is allowed to change private member functions of SBXxx classes?



================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:31
 
-  bool GetMemoryRegionAtIndex(uint32_t idx, SBMemoryRegionInfo &region_info);
+  void Reserve(size_t capacity);
 
----------------
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > This you can add, but std::vector implementations are quite efficient and I doubt this is really needed?
> > std::vector is as efficient as it can be without additional hints from the user, but reserve is a very useful optimization in general.
> Agreed but this doesn't need to be in the public API as there is no real need.
I'm not aware of the usual count of memory regions in dump files, but when I tested on a file with 140 regions the difference was significant
(if take into account all loops that do push_back without previous reserve).


================
Comment at: include/lldb/API/SBMemoryRegionInfoList.h:35
 
-  void Append(lldb::SBMemoryRegionInfoList &region_list);
+  void Append(const lldb::SBMemoryRegionInfo &region);
+
----------------
clayborg wrote:
> "const" means nothing to our objects as they have shared pointers or unique pointers which never change. 
Non-const argument version prevents passing r-value in this function. Another reason is readability.
Is this change braking API too?


================
Comment at: include/lldb/Target/Process.h:2084
   virtual Status
-  GetMemoryRegions(std::vector<lldb::MemoryRegionInfoSP> &region_list);
+  GetMemoryRegions(std::vector<lldb::MemoryRegionInfoUP> &region_list);
 
----------------
clayborg wrote:
> 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? 
Hah, I feel stupid now)


================
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)) {}
 
----------------
tatyana-krasnukha wrote:
> clayborg wrote:
> > 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.
> Private member functions cannot be changed too?
This is instead of private constructor with a raw pointer


================
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;
-}
----------------
clayborg wrote:
> 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.
Private member functions cannot be changed too?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55472