[PATCH] D51626: [clangd] Move buildStaticIndex() to SymbolYAML

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/index/SymbolYAML.cpp:193
+  auto Slab = symbolsFromYAML(Buffer.get()->getBuffer());
+  SymbolSlab::Builder SymsBuilder;
+  for (auto Sym : Slab)
----------------
this is deep copying the symbols for no reason

just `build(std::move(Slab))` below.
(I know you're just moving this code, but while you're here...)


================
Comment at: clang-tools-extra/clangd/index/SymbolYAML.h:44
 
+// Build an in-memory static index for global symbols from a YAML-format file.
+// The size of global symbols should be relatively small, so that all symbols
----------------
nit: can you just say "index file", and call the parameter "SymbolFile"?
See D51585 which adds another format (it can be detected by content sniffing, so no parameter needed)


================
Comment at: clang-tools-extra/clangd/index/SymbolYAML.h:47
+// can be managed in memory.
+std::unique_ptr<SymbolIndex> buildStaticIndex(llvm::StringRef YamlSymbolFile,
+                                              bool UseDex = true);
----------------
I'd prefer `loadIndex` for this function, what do you think?

"load" over "build" because much of the indexing work has already been done in producing the file, and in future potentially even more (serializing posting lists).
"static" is coupled to this index's role in ClangdMain, and the point of putting the function here is to allow other uses, so I'd prefer to drop that word.


https://reviews.llvm.org/D51626