Page MenuHomePhabricator

[clangd] Support dependent bases in type hierarchy
ClosedPublic

Authored by nridge on Mar 24 2019, 7:58 PM.

Details

Summary

Dependent bases are handled heuristically, by replacing them with the class
template that they are a specialization of, where possible. Care is taken
to avoid infinite recursion.

Event Timeline

nridge created this revision.Mar 24 2019, 7:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2019, 7:58 PM

This patch aims to implement the more proper solution suggested here.

I couldn't actually use CXXRecordDecl->getTemplateInstantiationPattern() as suggested, because the base-specifiers resolve to types, not declarations, but I believe what I did here implements the same intention (please let me know if I misunderstood).

sammccall accepted this revision.Apr 5 2019, 9:47 AM

LG, though I think your patch might be based on top of some uncommitted changes.

I noticed template names rather than type names are shown (not new in this patch) - we should fix that I think.

clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
384 ↗(On Diff #194002)

can we give this a more descriptive name like RecursiveHierarchyUnbounded?

408 ↗(On Diff #194002)

Sorry, I realize this isn't related to this patch, but I didn't see the final form of the previous one.

This should be WithName("S<0>"), ... Parents(AllOf(WithName("S<1>")), ...). S is the name of the template, not the name of the type.

Can you add a fixme?

413 ↗(On Diff #194002)

RecursiveHierarchyBounded

424–425 ↗(On Diff #194002)

this can be merged with the previous test case, using two named points.

449 ↗(On Diff #194002)

(in this case the printed name for the parent might end up looking odd - printing the spelled S<N - 1> is probably ideal but I don't really mind what we end up with, this is an edge case. Not for this patch, in any case)

452–453 ↗(On Diff #194002)

this patch doesn't seem to be against HEAD

This revision is now accepted and ready to land.Apr 5 2019, 9:47 AM
nridge updated this revision to Diff 194002.Apr 5 2019, 9:18 PM
nridge marked 6 inline comments as done.

Address review comments

nridge marked an inline comment as done.Apr 6 2019, 4:11 PM
nridge added inline comments.
clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
408 ↗(On Diff #194002)

(I looked into what it would take to fix this, and it seems like we need D59639, so I'm going to wait for that.)

sammccall accepted this revision.Apr 8 2019, 1:48 AM
sammccall added inline comments.
clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
408 ↗(On Diff #194002)

AFAICS D59639 is something subtly different - it prints the args of *specializations* as written in the source code, not instantiations. i.e. if you try to use this for vector<int>, it won't work as there's no specialization, you'll get vector<T> instead.

nridge marked an inline comment as done.Apr 11 2019, 8:48 PM
nridge added inline comments.
clang-tools-extra/unittests/clangd/TypeHierarchyTests.cpp
408 ↗(On Diff #194002)

I had another look, and you're right; I didn't appreciate that distinction.

I filed an issue to track implementing your suggestion. I plan to work on it.

In the meantime, could we get this patch committed?

Ping - could this be committed please?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2019, 6:37 PM