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