hhellyer marked 2 inline comments as done. hhellyer added a comment. In http://reviews.llvm.org/D20565#442373, @clayborg wrote:
> This looks good as long as SBMemoryRegionInfo and SBMemoryRegionInfoList are > ready only and will never have any setter functions. If we plan to ever have > the SBMemoryRegionInfo or SBMemoryRegionInfoList be modifiable, then we > should make each contain a std::unique_ptr instead of a std::shared_ptr. I’m not sure if there’s ever going to be a need for any setter methods on the SBMemoryRegionInfo classes. Can you suggest any use cases? In my mind they just reflect the state of the memory in the process, I'm not sure if they allow that state to be changed. For example a user wouldn’t ever directly create a new memory region by creating an SBMemoryRegionInfo. The process would allocate memory, memory map a file or load a module and a new one (might) be created. Other functions in lldb might cause the process to allocate or free memory and indirectly create or extend a memory region. Similarly changing a regions properties (e.g. making a region read only or changing it’s size) doesn’t necessarily make sense as those changes won't be reflected back in the process. If SBMemoryRegion had setters, what the desired behaviour would be? The original purpose of SBMemoryInfoList to take a snap shot of memory regions (instead of using GetNumRegions and GetRegionAtIndex which would give unreliable results if the process was allowed to continue) so I'm not sure how it would all fit together if memory regions could be created or updated especially if the process continued. I guess the other case is setting something on the memory region that changes the behaviour of lldb with respect to it or using an SBMemoryRegionInfo as an argument to another method. Then you might want to create a custom region and there may be some of those that make more sense. > So I propose the following changes: > > - make both SBMemoryRegionInfo and SBMemoryRegionInfoList contain > std::unique_ptr objects > - have their constructors always create a default object > - change copy constructors to copy the objects instead of sharing a reference > - change assignment operators to copy the objects instead of sharing a > reference I'll make the changes and upload a new patch, it certainly doesn't hurt to enable it as a possibility, I'm just not sure what the behaviour would be with modifiable memory regions. I can always go back to the shared pointer version. (Sorry for the delay in replying, it was a bank holiday here yesterday.) http://reviews.llvm.org/D20565 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
