ioeric added a comment. Thanks for the review!
Logic around CodeCompletionString is pulled into a separate file in https://reviews.llvm.org/D41450. I also propagate the new completion info to completion code. I am happy to split it out if it adds too much noise for the review. ================ Comment at: clangd/index/Index.h:92 + // Documentation including comment for the symbol declaration. + std::string Documentation; ---------------- sammccall wrote: > 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? As discussed offline, putting non-core stuff in an optional structure. ================ Comment at: clangd/index/Index.h:100 + std::string CompletionDetail; + // The placeholder text for function parameters in order. + std::vector<std::string> Params; ---------------- sammccall wrote: > 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. As discussed offline, we now store the whole snippet in the insertion text. ================ Comment at: clangd/index/SymbolCollector.cpp:61 + +std::string getDocumentation(const CodeCompletionString &CCS) { + // Things like __attribute__((nonnull(1,3))) and [[noreturn]]. Present this ---------------- sammccall wrote: > 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 I pulled `CodeCompletionString` handling logic into a separate file in `D41450`. 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
