[PATCH] D92480: [llvm] Unify interface of (ThreadSafe)?RefCountedBase

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:75-76
   RefCountedBase() = default;
+  // Copy and move constructors/assignments are no-ops as the RefCount isn't
+  // dictated by the class directly.
   RefCountedBase(const RefCountedBase &) {}
----------------
I can't quite understand this comment - perhaps you could try rephrasing it?


================
Comment at: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:79-80
+  RefCountedBase(RefCountedBase &&) {}
+  RefCountedBase &operator=(const RefCountedBase &) { return *this; }
+  RefCountedBase &operator=(RefCountedBase &&) { return *this; }
+
----------------
What does it mean to copy a ref counted object with a builtin ref count?

I would /guess/ maybe copies (or moves) of ref counted objects like this should only be valid when the ref count is 1 or maybe only 0? (since all the other users who might have an active reference certainly aren't getting a reference to the newly copied object implicitly)

Do you know which users which relied on any of the copy behavior? Maybe leave it as = default for now?

I guess it makes sense that the copy would have a zero ref count - same as if it were newly default constructed. But if no one's been using this effectively/at all already - maybe avoid creating this new behavior/changing the semantics here? What if we only allowed copies of objects with a zero ref count - is that sufficient for existing uses?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92480