[PATCH] D58341: [clangd] Index UsingDecls

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the explanation.

In D58341#1401365 <https://reviews.llvm.org/D58341#1401365>, @ilya-biryukov wrote:

> In D58341#1401295 <https://reviews.llvm.org/D58341#1401295>, @hokein wrote:
>
> > std::strcmp is a fair case here. Sema seems not returning using-decls as part of code completion results, it this an intended behavior?
>
>
> Yeah, I think it is. There's an explicit code path that takes the target decls of a using. Arguably, that's good if you to show signatures of the methods.
>
> > Is it possible for us to extend Sema to support it?
>
> We could, but then we'd loose the signatures of the targets functions, which is sad :-(
>
> > If we decide to provide using-decl results from index, I think we should make sure the code completion information (e.g. signature) is correct.
>
> The problem is that using-decls have multiple signatures. They can introduce more than one name into the scope, so the question is which one should we pick and how should we store them.


I think for most cases, the using-decl has only one shadow decl,  anyway it is out scope of this patch.

> In any case, it feels like any solution we can come up with would require storing using declarations in the index in one form or the other, so this patch definitely makes sense: it gives us hooks we can use to handle usings in clangd.



================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:1126
+TEST_F(SymbolCollectorTest, UsingDecl) {
+  auto completions = [](const Annotations &Test, SymbolSlab SS) {
+    class IgnoreDiagnostics : public DiagnosticsConsumer {
----------------
ilya-biryukov wrote:
> That's definitely too much setup for such a simple test.
> I thought it's possible to wire up a real index in the completion tests, but it seems that's not the case. So let's not bother to run an actual completion here, ignore my previous comment about adding a test.
> 
> I thought it's possible to wire up a real index in the completion tests, but it seems that's not the case. So let's not bother to run an actual completion here, ignore my previous comment about adding a test.

Adding completions to `SymbolCollectorTest` is overweight, but I think this is possible (and worthy) to add one to CodeCompleteTest without too much effort. We have `TU.index()` to build a real index.

I understand the problem better now, we are missing some decls from sema because we avoid deserialization in preamble, I think we should document it somewhere, can't think a better place, maybe at the completion test?


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:14
 #include "TestTU.h"
+#include "index/Index.h"
+#include "index/MemIndex.h"
----------------
These includes are not needed.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:57
 }
+MATCHER_P(CompletionQName, Name, "") { return (arg.Scope + arg.Name) == Name; }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
----------------
here as well.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58341