The parser gives implicit template instantiations to the action's HandleTopLevelDecls callback. This makes semantic highlighting highlight these templated functions multiple times. Fixed by filtering on if a Decl is an implicit function/variable/class instantiation. Also added a testcase to semantic highlighting on this.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 36329 Build 36328: arc lint + arc unit
Event Timeline
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
67 | We also want to skip TSK_ExplicitInstantiationDeclaration and TSK_ExplicitInstantiationDefinition here. template <class T> int foo(T) { ... } template int foo(int); // we'd rather not traverse these, highlightings will run into the same problems. Semantics I'm describing are roughly similar to isImplicitInstatiation(D) == !isExplicitInstantion(D), where isExplicitInstantiation is taken from CodeComplete.cpp. (If we ignore TSK_Undeclared, which, I believe, should never be encountered in decls passed to HandleTopLevelDecl). Please extract the helper from code complete and this one into a separate file (e.g. clangd/AST.h) and possibly implement one through the other |
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
67 |
I think there is a typo here, I believe you mean isImplicitInstantiation(D) == !isExplicitSpecialization(D) (in CodeComplete.cpp, it checks whether a Decl is an explicit specialization). | |
clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp | ||
260 | instead adding a highlighting test, could we add this testcase to ClangdUnitTests.cpp instead? |
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
67 | Yes, there's a typo thanks! |
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
67 | Still want me to move this and the helper from CodeComplete to AST.h as it isn't used anywhere else? (esp. when we can't implement them through of each other) |
the change looks good to me, leave the final approval to Ilya as he knows more context.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
143 | nit: clang-format, the length seems > 80 column. |
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
67 | Yes, it's better to share this code between codeComplete and ClangdUnit. It might pop up in more places and it's not trivial. | |
75 | Could we expose the following non-template function instead? bool isImplicitTemplateInstantiation(NamedDecl *); So that the users don't need to specify which Decl they are interested in (I believe there's no use for it anyway) |
clang-tools-extra/clangd/ClangdUnit.cpp | ||
---|---|---|
67 | Sorry, I mean share functions from the same place between code complete and clangd unit. |
Move isImplicitTemplateInstantiation and isExplicitTemplateSpecialization, also share implementation.
clang-tools-extra/clangd/AST.h | ||
---|---|---|
84 | Could you add a small comment with an example? /// Indicates \p D is a template instantiation implicitly generated by the compiler, e.g. /// template <class T> struct vector {}; /// vector<int> v; // 'vector<int>' is an implicit instantiation bool isImplicitTemplateInstantiation(const NamedDecl *D); /// Indicates \p D is an explicit template specialization, e.g. /// template <class T> struct vector {}; /// template <> struct vector<bool> {}; // <-- explicit specialization /// /// Note that explicit instantiations are NOT explicit specializations, albeit they look similar. /// template struct vector<bool>; // <-- explicit instantiation, NOT an explicit specialization. bool isExplicitTemplateSpecialization(const NamedDecl *D); | |
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
137 | Could you also check that:
template <> void f(bool) {}
template void f(bool);
template <class T> struct vector {}; template <class T> struct vector<T*> {}; // partial specialization, should be present
template <class T> T foo = 10; // (!) requires C++17 |
Added tests for making sure explicit specializations, explicit instantiations, partial instantiations, explicit declarations and template variables are in topLevelDecls. Also added a test to SemanticHighlightingTests to make sure that explicit instantiations are being traversed (so they aren't accidentaly removed from topLevelDecls).
Also added comment for the two added functions in AST.h.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
137 | Explicit instantiations are present in topLevelDecls though, otherwise RecursiveASTVisitor would not traverse them (so adding a test to make sure explicit instantiations are included in toplevel). Also adding a test to SemanticHighlighting to make sure that explicit instantiations are visited in that (is some other RecursiveASTVisitor usage I should add this to instead?) |
One important comment about somehow distinguishing multiple decls with the same name.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
137 | Yes, sorry, I forgot about the results of your investigation yesterday. | |
161 | Also add a full specialization (IIRC, they are represented completely differently in the AST): template <> struct V<bool> {}; | |
164 | Also add a partial and a full template specializations for the variable declaration: template <class T> int foo<T*> = 0; template <> int foo<bool> = 0; | |
171 | Is there a way to check we are seeing the explicit instantiations? (e.g. by looking at template arguments?) It's not clear whether multiple DeclNamed("foo") refer to the decls we expect. |
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
171 | Well in this case all the expressions in the top level should be in topLevelDecls so unless say void f(T) {} somehow starts duplicating while void f(bool) disappears from topLevelDecls they should all be visible. A thing we we could do is shuffle the order of the decls in the test so we never have two decls of the same name after each other (because the matcher cares about the order of the elements). Which should give us pretty high confidence we are getting the correct decls... Or maybe I'm misunderstanding you? |
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
171 | The tests should be easy to read and understand, we should definitely find a way to distinguish decls with different arguments. AllOf(DeclNamed("f"), WithTemplateArgs("<bool>")) Implementing the matcher is easy with printTemplateSpecializationArgs from AST.h |
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
85 | Could you wrap the result with < and >, add , as a separator? | |
91 | NIT: remove braces | |
97 | NIT: remove braces | |
186 | NIT: do not match WithTemplateArgs for non-template names (like i and d here)? | |
188 | NIT: add // FIXME: this should be '<T*>', not '<>' |
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
171 | Will fix the partial specialization in a separate CL |
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp | ||
---|---|---|
188 | Already have a fix for it (it's a 2 line fix without the tests, should I just add do it in a separate CL or add it to this?) |
LGTM, but please print the function template arguments uniformly before landing this (to avoid different forms of outputs inside the same test).
Could you add a small comment with an example?