[PATCH] D65510: [clangd] Fix implicit template instatiations appearing as topLevelDecls.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

One important comment about somehow distinguishing multiple decls with the same name.



================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:110
+    template<typename T>
+    void f(T) {}
+    void s() {
----------------
jvikstrom wrote:
> ilya-biryukov wrote:
> > Could you also check that:
> > 
> > 1.  explicit specializations are present
> > ```
> > template <>
> > void f(bool) {}
> > ```
> > 2. explicit instantiations are absent
> > ```
> > template void f(bool);
> > ```
> > 3. partial specializations are present (they should not be affected by this change, but testing those here seems appropriate)
> > ```
> > template <class T>
> > struct vector {};
> > 
> > template <class T>
> > struct vector<T*> {}; // partial specialization, should be present
> > ```
> > 4. template variables and classes are also handled:
> > ```
> > template <class T>
> > T foo = 10; // (!) requires C++17
> > ```
> 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?)
> 
Yes, sorry, I forgot about the results of your investigation yesterday.
Having them in top-level decls seems fine, just wanted to make sure we actually test it.
LG


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:134
+    template<class T>
+    struct V<T*> {};
+
----------------
Also add a full specialization (IIRC, they are represented completely differently in the AST):
```
template <>
struct V<bool> {};
```


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:137
+    template<class T>
+    T foo = T(10);
+    int i = foo<int>;
----------------
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;
```


================
Comment at: clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp:144
+  EXPECT_THAT(AST.getLocalTopLevelDecls(),
+              ElementsAre(DeclNamed("f"), DeclNamed("f"), DeclNamed("f"),
+                          DeclNamed("V"), DeclNamed("V"), DeclNamed("foo"),
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65510/new/

https://reviews.llvm.org/D65510