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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
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(),
----------------
kbobyrev wrote:
> 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!
+1 on not reporting these locations (we also don't report destructor decls in `refInDecl`).

> because IIUC this would prevent destructors/ctors from appearing in x-refs which might be a regression.

we don't use findExplicitReferences in XRefs, so it should be ok.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:601
+      // visited separately.
+      if (llvm::dyn_cast<CXXDestructorDecl>(E->getMemberDecl()))
+        return;
----------------
I think we should use `E->getFoundDecl()` here, could you add a test case for  calling base destructor explicitly, e.g. `derived->base::~base()`?


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:882
         "10: targets = {ns}\n"},
+       // CXX Constructor, destructor and operators.
+       {R"cpp(
----------------
could we simplify the newly-added test? I think a test only for explicit destructor calls is sufficient, I think.

```
void foo () {
  class Foo { ~Foo(); };
 Foo f;
 f.~Foo();
}
```


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:900
+               $8^Foo $9^f;
+               int $10^fourtyTwo = $11^f.$12^operator int();
+               $13^f.~ /*...*/ $14^Foo();
----------------
kbobyrev wrote:
> `$12` would not be captured if we simply do
> 
> ```
> if (E->getMemberNameInfo().getNamedTypeInfo())
>   return;
> ```
If we want to keep the test for operator int, just create a new test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72638