This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Enable textual fallback for go-to-definition on dependent names
ClosedPublic

Authored by nridge on Mar 19 2020, 1:29 PM.

Diff Detail

Event Timeline

nridge created this revision.Mar 19 2020, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2020, 1:29 PM
nridge marked an inline comment as done.Mar 19 2020, 1:39 PM
nridge added inline comments.
clang-tools-extra/clangd/XRefs.cpp
228

In the future, I can envision expanding this to return additional information, possibly the AST node itself. For now, I just started with a boolean.

nridge added a comment.Apr 7 2020, 6:49 PM

Review ping :)

nridge updated this revision to Diff 259160.Apr 21 2020, 9:51 PM

Rebase on top of D75479

Thanks for your patience!

clang-tools-extra/clangd/XRefs.cpp
154

shovelling this IsDependentName bool around seems a bit ad-hoc. The actual node doesn't live long enough, but can you make it an ASTNodeKind instead, and move the isDependentName check down to locateTextually where it makes sense to be that specific?

354

This sentence is hard to parse now, and also doesn't really justify the behaviour.
Suggest leaving it alone and adding:
// Exception: dependent names, because XYZ

570–572

the way the two (possible!) writes get combined is not obvious here.

I'd suggest using two different variables and writing IsDependent || IsNearbyIdentDependent in the usage below, if that's the intent.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
665–666

move these into the header so it can't be identifier-based heuristics? (And so the test doesn't change behaviour when we turn that on)

693

this has a loop but only one test...

780

this looks like a very close superset of TextualDependent, do we need both?

Again, these decls should move into the header I think.

nridge updated this revision to Diff 259483.Apr 22 2020, 10:56 PM
nridge marked 7 inline comments as done.

Address review comments

clang-tools-extra/clangd/XRefs.cpp
570–572

Good catch. There's actually no point letting this second locateASTReferent() call set IsDependentName, because IsDependentName only changes the outcome if the original location was an expanded token, but findNearbyIdentifier() bails in that case. So now I just pass nullptr instead.

(We could work harder and make "comment referencing nearby dependent name" work, but I think that's enough of an edge that I didn't bother.)

sammccall accepted this revision.Apr 24 2020, 6:38 AM

Great, thank you!

This revision is now accepted and ready to land.Apr 24 2020, 6:38 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 26 2020, 4:22 AM

This breaks tests on Windows: http://45.33.8.238/win/13855/step_9.txt

Please take a look and revert while you do if it takes a while to investigate.

This breaks tests on Windows: http://45.33.8.238/win/13855/step_9.txt

Please take a look and revert while you do if it takes a while to investigate.

@nridge hold off on the revert, I suspect this is our good friend -fdelayed-template-parsing and I'm about to nuke it in D78848. Committing now, let's see if that fixes it.

Yes, looks fixed by 6d7637d.