<table border="1" cellspacing="0" cellpadding="8">
    <tr>
        <th>Issue</th>
        <td>
            <a href=https://github.com/llvm/llvm-project/issues/151026>151026</a>
        </td>
    </tr>

    <tr>
        <th>Summary</th>
        <td>
            Change IntrusiveRefCntPtr API to reduce unclear object lifetimes
        </td>
    </tr>

    <tr>
      <th>Labels</th>
      <td>
            new issue
      </td>
    </tr>

    <tr>
      <th>Assignees</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Reporter</th>
      <td>
          jyknight
      </td>
    </tr>
</table>

<pre>
    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!)
</pre>
<img width="1" height="1" alt="" src="http://email.email.llvm.org/o/eJycVl1v4zoO_TXKC1HDkdt8POTB0w-gwJ3dizuDi92nQpEYW1NZyohS0vz7BWWn311gFwhQuJLIo6PDQyoi23nEjbj6Jq5uZiqnPsTNr9Ojt12fZttgTpufPcJRnUAs6nufYiZ7wL9wd-3TnymKRQ2WIBMasB7--OPv75CCUScY1CMSpB4hHD1G6u0ewg4o6x4i7jCi13ihQ_YJDYTtL9SJ4He2CUEHv8tkfVeJuhV1e584CfpkI7oTZK8dqgjHHj2cQoa9IhLyGkKEiClHzx_qJQ0v7IP1CSOkMK68zcz_pjAg7LLXyYYS4dhj6jFyCiGXEeHBoTfWdw-gzvGmtOetfN-IGu0BIxytc4DeQN5DUo_Wd0C9ipz0NSWFo4KigtYbjrhFrTLhf-XcDntntU3uxHxRilknu3UIuxgGvqM6vgZZiNfBHzCSDZ6gV_s9elDOQThM0PdOaaxgZL0lCB4Bn9Swd8gxxKK-DsPeOoz3npLyGhlMr0g0fEIs6ulXt4dgDRCmHyFHjd-VVx0DWb35BiHbv5XLKOQaxPKbqFuYNnQRRHMD42rDK2J58y5H6w1jurGq84GS1XTrO-sLKOUoFGT_J6wfUX_v4itcIOSdkC0oIoxJyFVVVWVZtrzCO95CnyJ8gV3U7b8xMatM_M5GSp8-KYECj0f4qAPWbFKPCA_vVfVwltW7W7KkrcOyRKiDNyykbYgxHFmdnx7hkuPFvA8e8Clh9Mq508WgrE_KejTg7A6THXAq159cl0eEXh0QtHIOI3Fm3YMJkHrrOwJnH4u-r53y3YVobj95FCEX7Y-fvNh9trjmX_NNLOoKfvZcEuxEjFV5aP-8n3KOlsA8Pjw7wkMhfqxJyxwbE3E0EeUNE-Ghs4eRFJXeuseHGhiJ1X15DpqK_OL5OSq4T0IuCX5lSlVVBWNejE2H7AxsEYqnYQS7-5igaUXTvicIlE6ZnwKsN1YrdrMC1rKdhccXPRSANvWgnv0N2PlVyhFZK2JRf1UYn_hPc_12U3ML5yIubzHe7S5EUD4UW3zlIYQIQjbzZn21upxcNiIMfAvtrFcFXQqFD89qIfZPrYbR2c7MsVfxPiGvRd3OK_gLTdb4yguDn672uYUWm3xlkgRKx0Bjz9LB4FZRQWw97NQhRI7Gncb6Tsi7UVUskC8znJvaeEOPGolUPJ1FNppxKhoLHIVb5rtIHIa5OF-qWMEUl5mQFfyTGxy_e5FYYQ255XCaR8R9uY8lylNniBi0zjHyNZoWdK98h5_YSwnHVZRCaeZvmX3PHgwhIhwwbsPIWsTf2capGPFpNLYvuGqu_yWa21HlOqJKeBfD8A88Fhirp1FY3Gf_l_O3T5aY3TdB2CkQdiEOGCeMeLbYaRRgb1nU5xFBNDeiuSmmzYbz7A_gVEpvYvhzgFJqL-dv6wq-B0rPVkh9qXl2A56cSrSvn_8YuVHHUYmUUBkWItpSWKPPE5bJ4TWsMntsEUzQecAy6SiCHtXBuhMYSzrkqDos4wZZ1pBlhJmQXo0n0yQW_FR2Qq6K02qM7P3uBEeelZYJHnbZudMDUHAHfO4ILwFAxZC9-TABlXEnJ1AJHCpKIwzP2Clv2f-SLSbnkOhlMmTFe95raRw4hZwLuZ6ZTWPWzVrNcDNfXjXLpp7LZtZvpGnWu_W2WSlVL9e7RX05313i1hjdrGWjmpndyFpe1Uu5mq_nV1JWyiyb5aKWSzSrldxKcVnjoKyrnDsMVYjdrBTVZn41r-Vi5tQWHZVJWkpWU1kVUvJgHTd86GKbOxKXtbOU6CVMssnh5vqrOjzXYBz97Tz8Tlo7E02zHN2mT2lfZjEeVe46m_q8rXQYhLzjdNOfi30MfFjIuwKShLybbnHYyP8EAAD__4BwJAw">