This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Have template template arguments target their referenced template decl
ClosedPublic

Authored by nridge on Aug 7 2020, 12:17 AM.

Details

Summary

As part of this change, DynTypedNode is enhanced to allow
storing a TemplateArgumentLoc.

Fixes https://github.com/clangd/clangd/issues/473

Diff Detail

Event Timeline

nridge created this revision.Aug 7 2020, 12:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 12:17 AM
nridge requested review of this revision.Aug 7 2020, 12:17 AM
hokein added a subscriber: hokein.Aug 7 2020, 12:45 AM

The code looks good.

I think we'd better to have some sort of tests in clang (rather than relying on clangd test). It seems that the only way to test it in clang is through ASTMatcher. Do you mind adding a templateArgumentLoc matcher as well? and add a test in unittests/ASTMatchers/ASTMatchersTraversalTest.cpp.
With that, I think it would be nice to have two patches, one for clang, the other is for clangd.

nridge updated this revision to Diff 284245.Aug 9 2020, 5:40 PM

Address review comments

nridge added a comment.Aug 9 2020, 5:41 PM

I've added the matcher and split the clang parts into D85621

hokein added inline comments.Aug 9 2020, 11:47 PM
clang-tools-extra/clangd/Selection.cpp
482

can we add a test for the Selection?

nridge updated this revision to Diff 284271.Aug 10 2020, 12:32 AM

Add Selection test

Also simplify the FindTarget test case a bit

hokein accepted this revision.Aug 10 2020, 1:00 AM
This revision is now accepted and ready to land.Aug 10 2020, 1:00 AM