[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as done.
hokein added inline comments.


================
Comment at: clangd/index/CMakeLists.txt:5
+
+add_clang_library(clangdIndex
+  Index.cpp
----------------
sammccall wrote:
> hmm, I'm not sure whether we actually want this to be a separate library. This means we can't depend on anything from elsewhere in clangd, and may force us to create more components.
> 
> e.g. if we want to pass contexts into the index, or if we want to reuse LSP data models from protocol.h.
> 
> Maybe we should make this part of the main clangd lib, what do you think?
As discussed offline, made it to the main clangd library.


================
Comment at: clangd/index/Index.cpp:34
+
+SymbolSlab::const_iterator SymbolSlab::find(const SymbolID& SymID) const {
+  return Symbols.find(SymID);
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > assert frozen? (and in begin())
> > We may not do the assert "frozen" in these getter methods -- as writers may also have needs to access these APIs (e.g. checking whether SymbolID is already in the slab before constructing and inserting a new Symbol).
> Right, I'd specifically like not to allow that if possible - it makes it very difficult to understand what the invariants are and what operations are allowed.
> 
> I think there's no such need now, right? If we want to add one in future, we can relax this check, or add a builder, or similar - at least we should explicitly consider it.
hmm, SymbolCollector has such need at the moment -- ` if (Symbols.find(ID) != Symbols.end()) return true;` to avoid creating a duplicated Symbol.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40897