| Issue |
151026
|
| Summary |
Change IntrusiveRefCntPtr API to reduce unclear object lifetimes
|
| Labels |
new issue
|
| Assignees |
|
| Reporter |
jyknight
|
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,
1. Reduce construction of `IntrusiveRefCntPtr` from raw pointers across the codebase, in favor of passing/returning `IntrusiveRefCntPtr` objects where necessary, and converting to `makeIntrusiveRefCnt` to construct new objects.
2. 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)` or `IntrusiveRefCntPtr<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 the `makeIntrusiveRefCnt` 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!)
![]()
_______________________________________________
llvm-bugs mailing list
llvm-bugs@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
