sammccall marked 4 inline comments as done. sammccall added inline comments.
================ 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" ---------------- kbobyrev wrote: > 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. Yup, that's right, thanks. (An intermediate version of the code used the overloaded new from allocator.h) ================ Comment at: clang-tools-extra/clangd/index/Ref.h:140 + SymbolID Symbol; + Ref R; + }; ---------------- kbobyrev wrote: > 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`? Oops, I agree but forgot to do this, thanks! 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
