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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
