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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/CMakeLists.txt:5
+
+add_clang_library(clangdIndex
+  Index.cpp
----------------
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?


================
Comment at: clangd/index/Index.cpp:34
+
+SymbolSlab::const_iterator SymbolSlab::find(const SymbolID& SymID) const {
+  return Symbols.find(SymID);
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40897