[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
This revision was automatically updated to reflect the committed changes.
Closed by commit rL340005: [clangd] Fetch documentation from the Index during
signature help (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50727?vs=160992=161189#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50727
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
EXPECT_TRUE(Results.Completions.empty());
}
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+ std::unique_ptr Index;
+ if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
MockFSProvider FS;
MockCompilationDatabase CDB;
IgnoreDiagnostics DiagConsumer;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer::Options Opts = ClangdServer::optsForTest();
+ Opts.StaticIndex = Index.get();
+
+ ClangdServer Server(CDB, FS, DiagConsumer, Opts);
auto File = testPath("foo.cpp");
Annotations Test(Text);
runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
return false;
return true;
}
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
Matcher Sig(std::string Label,
std::vector Params) {
@@ -1594,6 +1603,51 @@
ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
}
+TEST(SignatureHelpTest, IndexDocumentation) {
+ Symbol::Details DocDetails;
+ DocDetails.Documentation = "Doc from the index";
+
+ Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+ Foo0.Detail =
+ Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+ Foo1.Detail =
+ Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0, Foo1, Foo2})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+ AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/CodeComplete.h
===
--- clang-tools-extra/trunk/clangd/CodeComplete.h
+++ clang-tools-extra/trunk/clangd/CodeComplete.h
@@ -172,12 +172,11 @@
CodeCompleteOptions Opts);
/// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-const tooling::CompileCommand ,
-PrecompiledPreamble const *Preamble,
-StringRef Contents, Position Pos,
-IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileCommand ,
+ PrecompiledPreamble const *Preamble, StringRef Contents,
+ Position Pos, IntrusiveRefCntPtr VFS,
+ std::shared_ptr PCHs, SymbolIndex *Index);
// For index-based completion, we only consider:
// * symbols in namespaces or translation unit scopes (e.g. no class
Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp
@@ -177,15 +177,16 @@
auto PCHs = this->PCHs;
auto FS = FSProvider.getFileSystem();
- auto Action = [Pos, FS, PCHs](Path File, Callback CB,
-llvm::Expected IP) {
+ auto *Index = this->Index;
+ auto Action = [Pos, FS, PCHs, Index](Path File, Callback CB,
+ llvm::Expected IP) {
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
ilya-biryukov wrote:
> ioeric wrote:
> > hokein wrote:
> > > ioeric wrote:
> > > > ilya-biryukov wrote:
> > > > > ioeric wrote:
> > > > > > drive by: I think this should be `vlog` or `dlog`.
> > > > > Code completion also logs the number of results from sema, index,
> > > > > etc. using the `log()` call.
> > > > > The added log message looks similar, so trying to be consistent with
> > > > > the rest of the code in this file.
> > > > >
> > > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd
> > > > > rather leave it to a separate patch.
> > > > I'm not sure which level code completion log messages should be in, but
> > > > I think we should follow the guidelines in the logger documentation
> > > > instead of the existing uses.
> > > >
> > > > Quote from Logger.h
> > > > ```
> > > > // log() is used for information important to understanding a clangd
> > > > session.
> > > > // e.g. the names of LSP messages sent are logged at this level.
> > > > // This level could be enabled in production builds to allow later
> > > > inspection.
> > > >
> > > > // vlog() is used for details often needed for debugging clangd
> > > > sessions.
> > > > // This level would typically be enabled for clangd developers.
> > > > ```
> > > My recent experience of debugging some weird GotoDef issues tells me that
> > > log of index should be on production (since it is a non-trivial part in a
> > > clangd session), it would be really helpful to understand what is going
> > > on.
> > I agree that they are useful for debugging, but they might not be
> > interesting to end-users. And I think that's why there is `vlog`. Clangd
> > developers could use a different log level with `--log` flag.
> I agree that `vlog` is probably a better fit here, but still think we should
> change it across `CodeComplete.cpp` in a single commit, rather than partially
> apply the guidelines to different parts of the file.
>
> However, if everyone agrees we should use `vlog` here, happy to use `vlog`
> too and also send a patch to update all the rest of the usages.
> The situation I'm trying to avoid is:
> 1. We commit `vlog` here.
> 2. Someone argues that using `log` is actually better and we decide to not
> change other usages to `log`.
> 3. We end up with inconsistent choices across this file: `vlog` for signature
> help and `log` for code completion.
>
> But if there's an agreement that we should use `vlog` everywhere, more than
> happy to change it :-)
That sounds good to me. Thanks!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > ioeric wrote:
> > > > > drive by: I think this should be `vlog` or `dlog`.
> > > > Code completion also logs the number of results from sema, index, etc.
> > > > using the `log()` call.
> > > > The added log message looks similar, so trying to be consistent with
> > > > the rest of the code in this file.
> > > >
> > > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather
> > > > leave it to a separate patch.
> > > I'm not sure which level code completion log messages should be in, but I
> > > think we should follow the guidelines in the logger documentation instead
> > > of the existing uses.
> > >
> > > Quote from Logger.h
> > > ```
> > > // log() is used for information important to understanding a clangd
> > > session.
> > > // e.g. the names of LSP messages sent are logged at this level.
> > > // This level could be enabled in production builds to allow later
> > > inspection.
> > >
> > > // vlog() is used for details often needed for debugging clangd sessions.
> > > // This level would typically be enabled for clangd developers.
> > > ```
> > My recent experience of debugging some weird GotoDef issues tells me that
> > log of index should be on production (since it is a non-trivial part in a
> > clangd session), it would be really helpful to understand what is going on.
> I agree that they are useful for debugging, but they might not be interesting
> to end-users. And I think that's why there is `vlog`. Clangd developers could
> use a different log level with `--log` flag.
I agree that `vlog` is probably a better fit here, but still think we should
change it across `CodeComplete.cpp` in a single commit, rather than partially
apply the guidelines to different parts of the file.
However, if everyone agrees we should use `vlog` here, happy to use `vlog` too
and also send a patch to update all the rest of the usages.
The situation I'm trying to avoid is:
1. We commit `vlog` here.
2. Someone argues that using `log` is actually better and we decide to not
change other usages to `log`.
3. We end up with inconsistent choices across this file: `vlog` for signature
help and `log` for code completion.
But if there's an agreement that we should use `vlog` everywhere, more than
happy to change it :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
hokein wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > ioeric wrote:
> > > > drive by: I think this should be `vlog` or `dlog`.
> > > Code completion also logs the number of results from sema, index, etc.
> > > using the `log()` call.
> > > The added log message looks similar, so trying to be consistent with the
> > > rest of the code in this file.
> > >
> > > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather
> > > leave it to a separate patch.
> > I'm not sure which level code completion log messages should be in, but I
> > think we should follow the guidelines in the logger documentation instead
> > of the existing uses.
> >
> > Quote from Logger.h
> > ```
> > // log() is used for information important to understanding a clangd
> > session.
> > // e.g. the names of LSP messages sent are logged at this level.
> > // This level could be enabled in production builds to allow later
> > inspection.
> >
> > // vlog() is used for details often needed for debugging clangd sessions.
> > // This level would typically be enabled for clangd developers.
> > ```
> My recent experience of debugging some weird GotoDef issues tells me that log
> of index should be on production (since it is a non-trivial part in a clangd
> session), it would be really helpful to understand what is going on.
I agree that they are useful for debugging, but they might not be interesting
to end-users. And I think that's why there is `vlog`. Clangd developers could
use a different log level with `--log` flag.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
hokein added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
ioeric wrote:
> ilya-biryukov wrote:
> > ioeric wrote:
> > > drive by: I think this should be `vlog` or `dlog`.
> > Code completion also logs the number of results from sema, index, etc.
> > using the `log()` call.
> > The added log message looks similar, so trying to be consistent with the
> > rest of the code in this file.
> >
> > Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather
> > leave it to a separate patch.
> I'm not sure which level code completion log messages should be in, but I
> think we should follow the guidelines in the logger documentation instead of
> the existing uses.
>
> Quote from Logger.h
> ```
> // log() is used for information important to understanding a clangd session.
> // e.g. the names of LSP messages sent are logged at this level.
> // This level could be enabled in production builds to allow later inspection.
>
> // vlog() is used for details often needed for debugging clangd sessions.
> // This level would typically be enabled for clangd developers.
> ```
My recent experience of debugging some weird GotoDef issues tells me that log
of index should be on production (since it is a non-trivial part in a clangd
session), it would be really helpful to understand what is going on.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
ilya-biryukov wrote:
> ioeric wrote:
> > drive by: I think this should be `vlog` or `dlog`.
> Code completion also logs the number of results from sema, index, etc. using
> the `log()` call.
> The added log message looks similar, so trying to be consistent with the rest
> of the code in this file.
>
> Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave
> it to a separate patch.
I'm not sure which level code completion log messages should be in, but I think
we should follow the guidelines in the logger documentation instead of the
existing uses.
Quote from Logger.h
```
// log() is used for information important to understanding a clangd session.
// e.g. the names of LSP messages sent are logged at this level.
// This level could be enabled in production builds to allow later inspection.
// vlog() is used for details often needed for debugging clangd sessions.
// This level would typically be enabled for clangd developers.
```
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
ioeric wrote:
> drive by: I think this should be `vlog` or `dlog`.
Code completion also logs the number of results from sema, index, etc. using
the `log()` call.
The added log message looks similar, so trying to be consistent with the rest
of the code in this file.
Maybe we should turn all of them into `vlog` or `dlog`, but I'd rather leave it
to a separate patch.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
hokein added inline comments. Comment at: clangd/index/Index.h:65 public: + static llvm::Optional forDecl(const Decl ); + ilya-biryukov wrote: > hokein wrote: > > We already have this similar function in clangd/AST.h. > Thanks for pointing this out. > It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern > would probably be a somewhat broken layering, right? E.g. `Index.h` should > not directly depend on anything AST-specific Yes, I think we will not add any AST-specific stuff to `Index.h`, that's why we have `AST.h`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov added inline comments.
Comment at: clangd/CodeComplete.cpp:742
+llvm::DenseMap FetchedDocs;
+if (Index) {
+ LookupRequest IndexRequest;
hokein wrote:
> nit: do we want to log anything here? It may be useful for debug.
Definitely useful. Thanks!
Comment at: clangd/index/Index.h:65
public:
+ static llvm::Optional forDecl(const Decl );
+
hokein wrote:
> We already have this similar function in clangd/AST.h.
Thanks for pointing this out.
It's somewhat hard to find. WDYT about moving it to `Index.h`? The concern
would probably be a somewhat broken layering, right? E.g. `Index.h` should not
directly depend on anything AST-specific
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ioeric added inline comments.
Comment at: clangd/CodeComplete.cpp:755
+ });
+ log("SigHelp: requested docs for {0} symbols from the index, got {1} "
+ "symbols with non-empty docs in the response",
drive by: I think this should be `vlog` or `dlog`.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov updated this revision to Diff 160992.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Log on index request, remove FIXME that was addressed
- Remove SymbolID::forDecl, use existing helper instead
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
Files:
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
EXPECT_TRUE(Results.Completions.empty());
}
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+ std::unique_ptr Index;
+ if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
MockFSProvider FS;
MockCompilationDatabase CDB;
IgnoreDiagnostics DiagConsumer;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer::Options Opts = ClangdServer::optsForTest();
+ Opts.StaticIndex = Index.get();
+
+ ClangdServer Server(CDB, FS, DiagConsumer, Opts);
auto File = testPath("foo.cpp");
Annotations Test(Text);
runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
return false;
return true;
}
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
Matcher Sig(std::string Label,
std::vector Params) {
@@ -1590,6 +1599,51 @@
ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
}
+TEST(SignatureHelpTest, IndexDocumentation) {
+ Symbol::Details DocDetails;
+ DocDetails.Documentation = "Doc from the index";
+
+ Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+ Foo0.Detail =
+ Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+ Foo1.Detail =
+ Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0, Foo1, Foo2})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+ AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -171,12 +171,11 @@
CodeCompleteOptions Opts);
/// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-const tooling::CompileCommand ,
-PrecompiledPreamble const *Preamble,
-StringRef Contents, Position Pos,
-IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileCommand ,
+ PrecompiledPreamble const *Preamble, StringRef Contents,
+ Position Pos, IntrusiveRefCntPtr VFS,
+ std::shared_ptr PCHs, SymbolIndex *Index);
// For index-based completion, we only consider:
// * symbols in namespaces or translation unit scopes (e.g. no class
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -687,18 +687,23 @@
llvm::unique_function ResultsCallback;
};
-using ScoredSignature =
-std::pair;
+struct ScoredSignature {
+ // When set, requires documentation to be requested from the index with this
+ // ID.
+ llvm::Optional IDForDoc;
+ SignatureInformation Signature;
+ SignatureQualitySignals Quality;
+};
class SignatureHelpCollector final : public CodeCompleteConsumer {
-
public:
SignatureHelpCollector(const clang::CodeCompleteOptions ,
- SignatureHelp )
- : CodeCompleteConsumer(CodeCompleteOpts, /*OutputIsBinary=*/false),
+ SymbolIndex *Index, SignatureHelp )
+ : CodeCompleteConsumer(CodeCompleteOpts,
+
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
hokein accepted this revision.
hokein added a comment.
looks good, a few nits.
Comment at: clangd/CodeComplete.cpp:742
+llvm::DenseMap FetchedDocs;
+if (Index) {
+ LookupRequest IndexRequest;
nit: do we want to log anything here? It may be useful for debug.
Comment at: clangd/index/Index.h:65
public:
+ static llvm::Optional forDecl(const Decl );
+
We already have this similar function in clangd/AST.h.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50727 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov updated this revision to Diff 160647.
ilya-biryukov added a comment.
- run clang-format
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
Files:
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.cpp
clangd/index/Index.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
EXPECT_TRUE(Results.Completions.empty());
}
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+ std::unique_ptr Index;
+ if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
MockFSProvider FS;
MockCompilationDatabase CDB;
IgnoreDiagnostics DiagConsumer;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer::Options Opts = ClangdServer::optsForTest();
+ Opts.StaticIndex = Index.get();
+
+ ClangdServer Server(CDB, FS, DiagConsumer, Opts);
auto File = testPath("foo.cpp");
Annotations Test(Text);
runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
return false;
return true;
}
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
Matcher Sig(std::string Label,
std::vector Params) {
@@ -1590,6 +1599,51 @@
ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
}
+TEST(SignatureHelpTest, IndexDocumentation) {
+ Symbol::Details DocDetails;
+ DocDetails.Documentation = "Doc from the index";
+
+ Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+ Foo0.Detail =
+ Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+ Foo1.Detail =
+ Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0, Foo1, Foo2})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+ AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -21,6 +21,8 @@
#include
namespace clang {
+class Decl;
+
namespace clangd {
struct SymbolLocation {
@@ -60,6 +62,8 @@
// SymbolID can be used as key in the symbol indexes to lookup the symbol.
class SymbolID {
public:
+ static llvm::Optional forDecl(const Decl );
+
SymbolID() = default;
explicit SymbolID(llvm::StringRef USR);
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "Index.h"
+#include "clang/Index/USRGeneration.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/SHA1.h"
#include "llvm/Support/raw_ostream.h"
@@ -23,6 +24,13 @@
<< L.End.Line << ":" << L.End.Column << ")";
}
+llvm::Optional SymbolID::forDecl(const Decl ) {
+ llvm::SmallString<128> USR;
+ if (index::generateUSRForDecl(, USR))
+return llvm::None;
+ return SymbolID(USR);
+}
+
SymbolID::SymbolID(StringRef USR)
: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {}
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -171,12 +171,11 @@
CodeCompleteOptions Opts);
/// Get signature help at a specified \p Pos in \p FileName.
-SignatureHelp signatureHelp(PathRef FileName,
-const tooling::CompileCommand ,
-PrecompiledPreamble const *Preamble,
-StringRef Contents, Position Pos,
-IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+SignatureHelp
+signatureHelp(PathRef FileName, const tooling::CompileCommand ,
+
[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help
ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, kadircet.
Herald added subscribers: arphaman, mgrang, jkorous, MaskRay.
Sema can only be used for documentation in the current file, other doc
comments should be fetched from the index.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50727
Files:
clangd/ClangdServer.cpp
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.cpp
clangd/index/Index.h
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -826,11 +826,19 @@
EXPECT_TRUE(Results.Completions.empty());
}
-SignatureHelp signatures(StringRef Text) {
+SignatureHelp signatures(StringRef Text,
+ std::vector IndexSymbols = {}) {
+ std::unique_ptr Index;
+ if (!IndexSymbols.empty())
+Index = memIndex(IndexSymbols);
+
MockFSProvider FS;
MockCompilationDatabase CDB;
IgnoreDiagnostics DiagConsumer;
- ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+ ClangdServer::Options Opts = ClangdServer::optsForTest();
+ Opts.StaticIndex = Index.get();
+
+ ClangdServer Server(CDB, FS, DiagConsumer, Opts);
auto File = testPath("foo.cpp");
Annotations Test(Text);
runAddDocument(Server, File, Test.code());
@@ -845,6 +853,7 @@
return false;
return true;
}
+MATCHER_P(SigDoc, Doc, "") { return arg.documentation == Doc; }
Matcher Sig(std::string Label,
std::vector Params) {
@@ -1590,6 +1599,51 @@
ElementsAre(Sig("foo(T, U) -> void", {"T", "U"})));
}
+TEST(SignatureHelpTest, IndexDocumentation) {
+ Symbol::Details DocDetails;
+ DocDetails.Documentation = "Doc from the index";
+
+ Symbol Foo0 = sym("foo", index::SymbolKind::Function, "@F@\\0#");
+ Foo0.Detail =
+ Symbol Foo1 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#");
+ Foo1.Detail =
+ Symbol Foo2 = sym("foo", index::SymbolKind::Function, "@F@\\0#I#I#");
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+int foo(double);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(double) -> int", {"double"}), SigDoc("";
+
+ EXPECT_THAT(
+ signatures(R"cpp(
+int foo();
+// Overriden doc from sema
+int foo(int);
+// Doc from sema
+int foo(int, int);
+
+void test() {
+ foo(^);
+}
+ )cpp",
+ {Foo0, Foo1, Foo2})
+ .signatures,
+ ElementsAre(AllOf(Sig("foo() -> int", {}), SigDoc("Doc from the index")),
+ AllOf(Sig("foo(int) -> int", {"int"}),
+SigDoc("Overriden doc from sema")),
+ AllOf(Sig("foo(int, int) -> int", {"int", "int"}),
+SigDoc("Doc from sema";
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -21,6 +21,8 @@
#include
namespace clang {
+class Decl;
+
namespace clangd {
struct SymbolLocation {
@@ -60,6 +62,8 @@
// SymbolID can be used as key in the symbol indexes to lookup the symbol.
class SymbolID {
public:
+ static llvm::Optional forDecl(const Decl& D);
+
SymbolID() = default;
explicit SymbolID(llvm::StringRef USR);
Index: clangd/index/Index.cpp
===
--- clangd/index/Index.cpp
+++ clangd/index/Index.cpp
@@ -8,6 +8,7 @@
//===--===//
#include "Index.h"
+#include "clang/Index/USRGeneration.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/SHA1.h"
#include "llvm/Support/raw_ostream.h"
@@ -23,8 +24,16 @@
<< L.End.Line << ":" << L.End.Column << ")";
}
+llvm::Optional SymbolID::forDecl(const Decl ) {
+ llvm::SmallString<128> USR;
+ if (index::generateUSRForDecl(, USR))
+return llvm::None;
+ return SymbolID(USR);
+}
+
SymbolID::SymbolID(StringRef USR)
-: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {}
+: HashValue(SHA1::hash(arrayRefFromStringRef(USR))) {
+}
raw_ostream <<(raw_ostream , const SymbolID ) {
OS << toHex(toStringRef(ID.HashValue));
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -176,7 +176,8 @@
PrecompiledPreamble const *Preamble,
StringRef Contents, Position Pos,
IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHs);
+
