sammccall added inline comments.
================
Comment at: clangd/index/Index.h:92
+ // Documentation including comment for the symbol declaration.
+ std::string Documentation;
----------------
AFAIK this information isn't needed for retrieval/scoring, just for display.
LSP has `completionItem/resolve` that adds additional info to a completion
item. This allows us to avoid sending a bunch of bulky comments, most of which
will never be looked at.
In practice, there's nothing we particularly want to do differently for the
memory index: we have to load the data into memory, and so including a pointer
to it right away is no extra work.
However Symbol has two roles, and being the in-memory representation for
MemIndex is only secondary. Its primary role is defining the protocol between
indexes and clangd, including remote indexes where returning all documentation
*is* expensive.
One option is to have Symbol just have the "core stuff" that's suitable for
returning in bulk, and have an index method to retrieve more details that would
be a point lookup only. (Maybe this is just the getSymbol method we've thought
about). I'm not sure what it means for the data structure. OK if we discuss
offline?
================
Comment at: clangd/index/Index.h:99
+ // Detail about the symbol. For example, the result type of a function.
+ std::string CompletionDetail;
+ // The placeholder text for function parameters in order.
----------------
What are you planning to put here other than the return type of a function?
It's probably OK if we explicitly want this to be "whatever should go in the
LSP detail field" but we should think about whether there's something more
specific we can say.
================
Comment at: clangd/index/Index.h:100
+ std::string CompletionDetail;
+ // The placeholder text for function parameters in order.
+ std::vector<std::string> Params;
----------------
How are you planning to use this?
This seems to be related to the completion text/edits. We had some early
discussions about whether we'd encode something like CodeCompletionString, or
LSP snippets, or something else entirely.
Honestly it would be great to have a doc describing this mapping between source
-> index -> LSP for completion data.
================
Comment at: clangd/index/SymbolCollector.cpp:61
+
+std::string getDocumentation(const CodeCompletionString &CCS) {
+ // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this
----------------
it seems we'll want to share the(some?) doc logic between hover, AST-based
complete, and indexing... See D35894 (which is ... complicated, no idea if
it'll land soon).
Among other things:
- we may not want to make the logic too elaborate until we're able to merge
interfaces
- we may want to consume AST nodes rather than CCS in the long run
================
Comment at: clangd/index/SymbolCollector.cpp:67
+ if (AnnotationCount > 0) {
+ Result += "Annotation";
+ if (AnnotationCount == 1) {
----------------
Should these annotations go at the end?
================
Comment at: clangd/index/SymbolCollector.cpp:90
+
+void ProcessChunks(const CodeCompletionString &CCS, Symbol *Sym) {
+ for (const auto &Chunk : CCS) {
----------------
nit: lowercase
================
Comment at: clangd/index/SymbolCollector.cpp:90
+
+void ProcessChunks(const CodeCompletionString &CCS, Symbol *Sym) {
+ for (const auto &Chunk : CCS) {
----------------
sammccall wrote:
> nit: lowercase
How does this relate to the code in AST-based completion?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41345
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
