[clang][Index] Introduce a TemplateParm SymbolKind
ClosedPublic
Actions

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

Details

Reviewers

sammccall
hokein

Commits

rG84240e0db8ab: [clang][Index] Introduce a TemplateParm SymbolKind

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

Repository: rG LLVM Github Monorepo

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

Herald added subscribers: cfe-commits, usaxena95, arphaman and 3 others. · View Herald Transcript

kadircet added a parent revision: D73693: [clang][DeclPrinter] Implement visitors for {TemplateType,NonTypeTemplate}Parms.Jan 30 2020, 5:20 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.

Harbormaster failed remote builds in B45338: Diff 241419!Jan 30 2020, 5:45 AM

kadircet added a reviewer: hokein.Feb 5 2020, 2:38 AM

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?

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

Harbormaster failed remote builds in B45925: Diff 243091!Feb 7 2020, 12:19 AM

ping :)

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.