[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)

[Alex Langford via lldb-commits]

================
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
 
 void WatchpointResource::AddOwner(const WatchpointSP &wp_sp) {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
-  m_owners.Add(wp_sp);
+  m_owners.push_back(wp_sp);
 }
 
 void WatchpointResource::RemoveOwner(WatchpointSP &wp_sp) {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
-  m_owners.Remove(wp_sp);
+  const auto &it = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+  if (it != m_owners.end())
+    m_owners.erase(it);
 }
 
 size_t WatchpointResource::GetNumberOfOwners() {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
-  return m_owners.GetSize();
+  return m_owners.size();
 }
 
 bool WatchpointResource::OwnersContains(WatchpointSP &wp_sp) {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
-  return m_owners.Contains(wp_sp);
+  const auto &it = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+  if (it != m_owners.end())
+    return true;
+  return false;
 }
 
 bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
-  return m_owners.Contains(wp);
+  for (WatchpointCollection::const_iterator it = m_owners.begin();
+       it != m_owners.end(); ++it)
+    if ((*it).get() == wp)
+      return true;
+  return false;
 }
 
 WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
   std::lock_guard<std::recursive_mutex> guard(m_owners_mutex);
-  assert(idx < m_owners.GetSize());
-  if (idx >= m_owners.GetSize())
+  lldbassert(idx < m_owners.size());
+  if (idx >= m_owners.size())
     return {};
 
-  return m_owners.GetByIndex(idx);
+  return m_owners[idx];
----------------
bulbazord wrote:

Well, I'm not entirely sure how we're going to want to access the Owners list, so I'd like to understand more how you plan on using it. If you want to do something like `wp_resource_sp->Owners()[2]` then this method makes sense to keep around, but I'd also be curious to know why you would want to access the 3rd owner directly. What I would like to avoid is writing a loop like this:
```
for (int i = 0; i < wp_resource_sp.GetNumberOfOwners(); i++) {
  WatchpointSP wp_sp = wp_resource_sp->GetOwnerAtIndex(i);
  // Use wp_sp
}
```
The two main reasons I am not a fan of this pattern:
1. As you pointed out, one would need to grab the `wp_resource_sp` owner mutex to do this safely since another thread may call `RemoveOwner` in the middle of this loop. I can't say I'm a fan of exposing the vector primarily because it would mean we'd have to expose the mutex too.
2. Off-by-one errors are fairly easy to put in by mistake. Doubly so if we ever iterate in reverse (i >0 vs i >=0 for the termination condition).

If we don't need to be able to access any of the owners at an arbitrary index, I would suggest removing it so nobody can write that kind of loop. WDYT?

https://github.com/llvm/llvm-project/pull/68845