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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to