-
Notifications
You must be signed in to change notification settings - Fork 14.8k
Description
The way IntrusiveRefCntPtr
is used in LLVM today makes the ownership of such reference-counted objects quite confusing.
It is entirely unclear when you pass, or return, a reference or pointer to a refcounted object to some function, whether you're lending a pointer, or whether the receiver will end up taking shared ownership of the object. And, because IntrusiveRefCntPtr
is implicitly constructible from a raw pointer, such conversions happen all over the place.
As one example, CompilerInstance
has:
void setSourceManager(SourceManager *Value) {
SourceMgr = Value;
}
And DiagnosticsEngine
also has
void setSourceManager(SourceManager *SrcMgr) {
/* assert(...) */
SourceMgr = SrcMgr;
}
Yet, the first implicitly constructs a new IntrusiveRefCntPtr to take shared ownership of the SourceManager, while the second is borrowing the SourceManager, relying upon externally-maintained lifetime.
Then we have callers which do things like Clang->setSourceManager(&AST->getSourceManager());
. This is using an API which returns a reference, taking its address, and then giving that pointer to a CompilerInstance, which takes shared-ownership. It's just...odd.
It could be clearer if CompilerInstance::setSourceManager actually indicated that it took ownership, with a function signature of void setSourceManager(IntrusiveRefCntPtr<SourceManager> Value)
.
For another example, see #139584, where my inclination to clean this up came from.
I plan to,
- Reduce construction of
IntrusiveRefCntPtr
from raw pointers across the codebase, in favor of passing/returningIntrusiveRefCntPtr
objects where necessary, and converting tomakeIntrusiveRefCnt
to construct new objects. - Once that's cleaned up, keep the issue from reoccurring: change IntrusiveRefCntPtr's API to make construction from raw pointer more verbose, requiring an explicit
IntrusiveRefCntPtr<X>::createFromNewPtr(x)
orIntrusiveRefCntPtr<X>::createFromExistingPtr(x)
. The former requires a new object (refcount==0
), and the latter requires an object with refcount>0. Most callers should just use themakeIntrusiveRefCnt
wrapper, instead of either of these. And the latter will be documented as heavily discouraged, since it causes ownership confusion.
(This certainly won't fully solve lifetime confusion around shared ownership, but at least it can be substantially less confusing than it is today!)