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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
