[PATCH] D79950: [clangd] Avoid wasteful data structures in RefSlab::Builder

Mon May 18 02:05:58 PDT 2020

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

LGTM with a couple of nits.

Thank you for the patch, this is a good idea!

================
Comment at: clang-tools-extra/clangd/index/Ref.cpp:11
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/StringSaver.h"
----------------
This defines `BumpPtrAllocator`, right? I think it should be included in `clang-tools-extra/clangd/index/Ref.h`, not here if the intent is to explicitly include used headers. Same for `StringSaver.h`, this source file does not use any additional variables of these types.

================
Comment at: clang-tools-extra/clangd/index/Ref.h:140
+      SymbolID Symbol;
+      Ref R;
+    };
----------------
nit: I know you like the short names, but with `R` I had to get back to `Entry` definition each time I saw it (I was thinking "wait, `R`? is there an `L` somewhere here?". Maybe it's just me, but one-letter fields seem scary. Maybe `Reference`?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79950