This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Refine the way of checking a declaration is referenced by the written code.
ClosedPublic

Authored by hokein on Dec 3 2018, 2:09 AM.

Details

Summary

The previous solution (checking the AST) is not a reliable way to
determine whether a declaration is explicitly referenced by the source
code, we are still missing a few cases.

Event Timeline

hokein created this revision.Dec 3 2018, 2:09 AM

The idea of using the AST-based approach here was really nice, it was less expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the source locations.

What are the cases we're trying to filter out? Only implicit constructor or anything else?

unittests/clangd/XRefsTests.cpp
1228

I'm missing what does this test actually tests.
The absence of implicit references (I guess constructor expressions)?

1259

Shouldn't the usings always be qualified? Isn't this a compiler error?

hokein updated this revision to Diff 177190.Dec 7 2018, 5:39 AM
hokein marked 4 inline comments as done.

Update based on our offline discussion.

hokein added a comment.Dec 7 2018, 5:41 AM

What are the cases we're trying to filter out? Only implicit constructor or anything else?

I think implicit constructor is the only case we care about. We also looked through the caller implementation of handleDeclOccurence, it is safe to assume it.

unittests/clangd/XRefsTests.cpp
1228

This test tests the cases where symbol is being marked implicit incorrectly, which will result in no result in xref.

1259

Yes, this is a compiler error, but clang's error recovery can handle it. Fixed.

ilya-biryukov accepted this revision.Dec 13 2018, 2:20 AM

LGTM. Many thanks for the fix, really important to get those cases right.

unittests/clangd/XRefsTests.cpp
1228

Thanks for clarification! The tested cases were so simple that I though they worked before.
(Which shows this change is very important, IMHO)

This revision is now accepted and ready to land.Dec 13 2018, 2:20 AM
This revision was automatically updated to reflect the committed changes.