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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
