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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits