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

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom marked an inline comment as done.
jvikstrom added inline comments.


================
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"),
----------------
ilya-biryukov wrote:
> jvikstrom wrote:
> > ilya-biryukov wrote:
> > > 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.
> > 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.
> > I could add a gtest matcher that also checks the types but I think that would become a pretty large matcher (would have to handle FunctionDecls with template arguments, VarDecls, TemplateClassSpecializationDecls etc.) and I'm not sure it would really add anything to the tests.
> > 
> > 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?
> The tests should be easy to read and understand, we should definitely find a way to distinguish decls with different arguments.
> Adding a matcher for template args should be easy enough:
> ```
> AllOf(DeclNamed("f"), WithTemplateArgs("<bool>"))
> ```
> 
> Implementing the matcher is easy with `printTemplateSpecializationArgs` from `AST.h`
Will fix the partial specialization in a separate CL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65510