[PATCH] D72638: [clangd] Fix rename for explicit destructor calls

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Going to investigate few interesting cases of `findExplicitReferences` not working as intended and opting out for a clean approach that prevents duplicates from getting into the reference set.



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:620
+      if (llvm::dyn_cast<CXXDestructorDecl>(E->getMemberDecl()))
+        NameLocation = E->getEndLoc();
       Refs.push_back(ReferenceLoc{E->getQualifierLoc(),
----------------
kadircet wrote:
> instead of patching the source location for destructors. we should probably not report anything for them in here, as they will be reported correctly as typelocs.
> 
> So you can check for `E->getMemberNameInfo().getNamedTypoInfo()` and bail out if its non-null, which means this is a constructor/destructor/operator with a typeloc that will be visited separately and result in the same ref. That way we would also get rid of duplicate refs being reported by `findExplicitReferences`, API doesn't mention anything about those but most of the callers would fail in the presence of duplicates, therefore I think we shouldn't be reporting duplicates. (for example the main visitor actually makes sure typelocs are not visited twice).
> 
> also could you add unittests for constructor and operator cases as well to make sure we get exactly one reference for those as well.
> instead of patching the source location for destructors. we should probably not report anything for them in here, as they will be reported correctly as typelocs.

Yes, I thought about that, but I decided against it because IIUC this would prevent destructors/ctors from appearing in x-refs which might be a regression. Also, this might be incorrect for cases like this one:

```
class Bar {};

class Foo {
public:
  operator Bar() const { return Bar(); }
};

int main() {
  Foo foo;
  Bar bar = foo.operator Bar();
}
```

Here, if we bail out on `foo.operator Bar()` we would miss the reference to the `operator Bar()` which we might want to preserve.

Actually, looking at the `FindTargets` unittests, there is something interesting happening: references to constructor have the constructor as the target, but destructor references target typename itself.

Another thing that is happening is that the following code has two references of bar at the same location:

```
class Foo {
public:
  operator $0^$1^Bar() const { return Bar(); }
};
```

I'm on it right now.

> also could you add unittests for constructor and operator cases as well to make sure we get exactly one reference for those as well.

Sure!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72638