[PATCH] D64312: [clangd] Simpler/more compact in-memory representation of RefSlab/Builder

[Haojian Wu via Phabricator via llvm-commits]

hokein added a comment.

just some nits to make the code more understandable.



================
Comment at: clang-tools-extra/clangd/index/Ref.cpp:36
 void RefSlab::Builder::insert(const SymbolID &ID, const Ref &S) {
-  auto &M = Refs[ID];
-  if (M.count(S))
-    return;
-  Ref R = S;
-  R.Location.FileURI =
-      UniqueStrings.save(R.Location.FileURI).data();
-  M.insert(std::move(R));
+  Refs.emplace_back(ID, S);
+  Refs.back().second.Location.FileURI =
----------------
before this changes, we were doing deduplications on the fly, now we deduplicate them only at the final/reducing stage.

Note that with this patch, `clangd-indexer` will be OOMed when indexing large scale projects like chromium, but I think `clangd-indexer` is not a tool we'd recommend users to use, that's probably fine? 


================
Comment at: clang-tools-extra/clangd/index/Ref.cpp:60
+  // Count distinct symbols and refs so we can size arrays correctly.
+  std::vector<bool> NewSymbol(Refs.size()), NewRef(Refs.size());
+  NewSymbol.front() = true;
----------------
nit: explicitly set the default value to `false`;

could we split the two variables into two lines? I felt inlining them into one line hurts the code readability (I have thought `NewRef` is one of the constructor parameters).


================
Comment at: clang-tools-extra/clangd/index/Ref.cpp:90
+      continue;
+    new (RefsEnd++) Ref(Refs[I].second);
+    if (NewSymbol[I])
----------------
nit: we have two `second`s in the loop body, they means different kinds (I have to go-back-and-forth to understand the code here). Could we explicitly make `StoredSymbol` a structure?


================
Comment at: clang-tools-extra/clangd/index/Ref.h:121
 
+class RefSlab::const_iterator
+    : public llvm::iterator_facade_base<
----------------
could we have some documentations here?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64312/new/

https://reviews.llvm.org/D64312