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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
