[Lldb-commits] [PATCH] D131996: Use a SmallPtrSet rather than a SmallVector in ClusterManager.

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/include/lldb/Utility/SharedCluster.h:52
   ClusterManager() : m_objects() {}
-
-  llvm::SmallVector<T *, 16> m_objects;
+  llvm::SmallPtrSet<T *, 16> m_objects;
   std::mutex m_mutex;
----------------
JDevlieghere wrote:
> labath wrote:
> > JDevlieghere wrote:
> > > I assume pointers cannot be modified once they're in the set. Can this be `const T *`? 
> > Did you mean pointers temselves, or the objects they point to? The pointers are automatically const (you can't modify the map copy in-place), and if someone wanted to store const objects, he could always instantiate this class with `const T` instead.
> I meant the latter. You could indeed have an instance of this class with a `const T`, but that has a different meaning. My point was that the `m_objects` could always store const objects (even though it's somewhat counter intuitive that you can delete a const pointer). Anyway, it's no big deal, and I guess not making this `const T*` allows you to have an actual `const T` template argument while otherwise we'd need `const std::remove_const<T>` to make that case work, which obviously isn't worth it. 
Ok, I see what you mean. That sounds reasonable, and I don't think the remove_const thingy would be necessary, as it should be possible add multiple `const` qualifiers in an idempotent manner.

Also agree, its not a big deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131996