[PATCH] D40548: [clangd] Symbol index interfaces and index-based code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/ClangdIndex.h:1
+//===--- ClangdIndex.h - Symbol indexes for clangd.---------------*- C++-*-===//
+//
----------------
sammccall wrote:
> nit: `Clangd` prefix doesn't do much.
> I'd suggest `Index.h` for the interface (including Symbol), and `MemIndex.h` for the in-memory implementations?
I'll do the renaming when D40897  is landed since it defines Symbol and would probably create `Index.h` as well.


================
Comment at: clangd/ClangdIndex.h:50
+// relatively small symbol table built offline.
+class InMemoryIndexSourcer {
+public:
----------------
sammccall wrote:
> As discussed offline - the lifetime of the contained symbols and the operations on the index derived from it need to be carefully considered.
> 
> Involving inheritance here seems likely to make these tricky interactions even trickier.
> 
> Would suggest:
>  - a concrete class (`SymbolCache`? `FileSymbols`?) to hold a collection of symbols from multiple files, with the ability to iterate over them and replace all the symbols from one file at once
>  - a concrete class `MemIndex` that can be constructed from a sequence of symbols and implements `Index`
> 
> You probably want to make them both immutable with snapshot semantics, or have a reader-writer lock that spans both.
The current revision implements a `FileSymbols` with the snapshot semantics. Not entirely sure if this is what you are looking for. Let me know...


================
Comment at: clangd/ClangdIndex.h:72
+   /// collected.
+   void update(PathRef Path, ASTContext &Context,
+               std::shared_ptr<Preprocessor> PP,
----------------
sammccall wrote:
> The AST -> symbol conversion doesn't seem to have much to do with the rest of the class - we can move this to a free function I think, which would give the class a narrower responsibility.
Moved the conversion code to ClangdUnit.cpp


================
Comment at: clangd/CodeComplete.cpp:378
+        // FIXME: output a warning to logger if there are results from sema.
+        return qualifiedIdCompletionWithIndex(*Index, S, **OptSS, &Items);
+      }
----------------
sammccall wrote:
> I don't like the idea of doing index lookups from the middle of a sema callback!
> 
> Can we just record the CCContext  in the Collector instead, and do this work afterwards?
Good point!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40548