Page MenuHomePhabricator

[clang][Index] Introduce a TemplateParm SymbolKind
ClosedPublic

Authored by kadircet on Jan 30 2020, 5:15 AM.

Details

Summary

Currently template parameters has symbolkind Unknown. This patch
introduces a new kind TemplateParm for templatetemplate, templatetype and
nontypetemplate parameters.

Also adds tests in clangd hover feature.

Diff Detail

Event Timeline

kadircet created this revision.Jan 30 2020, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2020, 5:15 AM

Unit tests: fail. 62337 tests passed, 1 failed and 838 were skipped.

failed: libc++.std/containers/sequences/array/array_creation/to_array.fail.cpp

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

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

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Libclang changes need tests I think.

clang-tools-extra/clangd/Protocol.cpp
265

this seems kind of dubious, maybe worth a comment?
(If we had the decl here we'd distinguish between type and non-type, right?)

clang-tools-extra/clangd/unittests/HoverTests.cpp
586

I do think we're going to want to have HI.Type = "typename" here, maybe with some special-case in the rendering.

(Especially with concepts where the TTP can be constrained as Integral or something)

Fine to leave for now but FIXME somewhere?

clang/include/clang-c/Index.h
6257 ↗(On Diff #241419)

As far as hover goes, calling this "template parameter" and making int/class the "type" is cute and fits well.

But I worry we're not exposing enough info here and at the libclang level (and I guess Index) we should be distinguishing the 3 cases. WDYT?

kadircet updated this revision to Diff 243091.Feb 6 2020, 11:59 PM
kadircet marked 2 inline comments as done.
  • As discussed offline, do not expose new symbol kinds to libclang
  • Set types for templateparms in HoverInfo.
kadircet marked an inline comment as done.Feb 7 2020, 12:05 AM
kadircet added inline comments.
clang-tools-extra/clangd/Protocol.cpp
265

we have all 3 of them now

hokein accepted this revision.Feb 14 2020, 12:39 AM
hokein added inline comments.
clang-tools-extra/clangd/CodeComplete.cpp
98

I think these two should belong to CompletionItemKind::TypeParameter?

clang/lib/Index/IndexSymbol.cpp
360

nit: could we move these newly-added lines above the case Decl::ClassTemplatePartialSpecialization:. I think we usually put the llvm_unreachable cases at the end of switch statement..

This revision is now accepted and ready to land.Feb 14 2020, 12:39 AM
kadircet marked 3 inline comments as done.Feb 14 2020, 4:20 AM
kadircet added inline comments.
clang-tools-extra/clangd/CodeComplete.cpp
98

right, moved the ones below but forgot to move these :D thanks!

clang/lib/Index/IndexSymbol.cpp
360

yes but this one is specially, there's also a default label below.

Also the unreachable ones are for making sure certain valid types don't arrive in here rather than checking for invalid types are passed.
We usually put the unreachable to the end if it is protecting against invalid usages, I believe.

kadircet updated this revision to Diff 244619.Feb 14 2020, 4:20 AM
kadircet marked an inline comment as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.