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

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 11 08:55:39 PST 2017

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 {
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!

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list