This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Show template arguments in type hierarchy when possible
ClosedPublic

Authored by nridge on Dec 15 2019, 10:18 PM.

Event Timeline

nridge created this revision.Dec 15 2019, 10:18 PM

I think this requires changes in other places too, for example when querying index for the children we rather want to query using the symbolid of template pattern, not the instantiation.

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

instead of doing the traversal twice, can we just sent both pattern and instantiation here, and then prefer the specializationdecl instead of just selecting decls[0] below ?

nridge updated this revision to Diff 234446.Dec 17 2019, 7:53 PM

Address review comments

nridge marked an inline comment as done.Dec 17 2019, 7:54 PM

I think this requires changes in other places too, for example when querying index for the children we rather want to query using the symbolid of template pattern, not the instantiation.

Good catch! I added a couple of additional test cases to exercise this.

kadircet added inline comments.Dec 18 2019, 1:34 AM
clang-tools-extra/clangd/XRefs.cpp
693

maybe just

const Decl *D = Decls.front()
for(const auto *C : Decls) {
  if(isa<ClassTempl...>(C)) {
    D = C;
    break;
  }
}
771

nit: no need for braces

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
418

what about making use of template pattern in case of invalid instantiations?

as type hierarchy tries to provide information regarding bases and children, I think it is more important to show those instead of template instantiation arguments.

464

can you also add a case for deriving from explicit (partial) specialization and type hierarchy on instantiation of a different pattern?
e.g.

template <typename T> class X {};
template <typename T> class X<T*> {};

struct Child : X<int*> {};

X<int> fo^o;
kadircet added inline comments.Dec 19 2019, 2:59 AM
clang-tools-extra/clangd/XRefs.cpp
693

now you can use explicitReferenceTargets instead, with only DeclRelation::Underlying set in the mask.

nridge updated this revision to Diff 234784.Dec 19 2019, 2:19 PM
nridge marked 4 inline comments as done.

Address most review comments

nridge marked an inline comment as done.Dec 19 2019, 2:20 PM
nridge added inline comments.
clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
418

Is there a way to query a ClassTemplateSpecializationDecl for whether instantiating its base specifier failed?

Or, should we fall back on the template pattern any time bases() is empty?

kadircet added inline comments.Jan 3 2020, 1:42 AM
clang-tools-extra/clangd/XRefs.cpp
140

this will result in changes in behavior for other functionality (it is unfortunate if no tests regressed), for example findReferences did work on template patterns rather than instantiations now it won't receive pattern results when triggered on instantiations. whether we want that or not is up for debate, but definitely not part of this patch.

clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
418

you can check for CTSD->isInvalidDecl() and fallback to pattern in such cases.

nridge updated this revision to Diff 236674.Jan 7 2020, 1:18 PM
nridge marked 3 inline comments as done.

Address review comments

Unit tests: pass. 61304 tests passed, 0 failed and 736 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

kadircet added inline comments.Jan 8 2020, 12:39 AM
clang-tools-extra/clangd/XRefs.cpp
688

instead of creating a new function, could you rather inline that ? it is the only call site and almost the same with getDeclAtPosition.
the comment above (with little modifications) should suffice to explain why getDeclAtPosition is not used here.

nridge updated this revision to Diff 237186.Jan 9 2020, 1:35 PM

Address latest review comment

Unit tests: pass. 61729 tests passed, 0 failed and 779 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

kadircet accepted this revision.Jan 10 2020, 1:36 AM

Thanks, LGTM!

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

nit: redundant braces

This revision is now accepted and ready to land.Jan 10 2020, 1:36 AM
nridge updated this revision to Diff 237572.Jan 12 2020, 7:31 PM
nridge marked an inline comment as done.

Address nit

This revision was automatically updated to reflect the committed changes.

Unit tests: pass. 61774 tests passed, 0 failed and 780 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml