[PATCH] D41178: [clangd] Construct SymbolSlab from YAML format.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice! just nits



================
Comment at: clangd/index/SymbolFromYAML.cpp:32
+// supported in YAML I/O.
+struct NPArray {
+  NPArray(IO &) {}
----------------
what does NP stand for?


================
Comment at: clangd/index/SymbolFromYAML.cpp:44
+
+  std::vector<uint8_t> Value;
+};
----------------
Nit: for readability, I suggest you encode as a hex string instead.

We don't yet have operator<< for SymbolID, but I think we should, and it should be consistent.
So preferably define `operator<<` for SymbolID, and use it here, rather than just defining everything here.


================
Comment at: clangd/index/SymbolFromYAML.h:1
+//===--- SymbolFromYAML.h ----------------------------------------*- C++-*-===//
+//
----------------
nit: `SymbolFromYAML.h` seems slightly off for the file, because you support both conversions (and also multiple symbols)

Consider just `YAML.h`?


================
Comment at: clangd/index/SymbolFromYAML.h:8
+//
+//===----------------------------------------------------------------------===//
+
----------------
File comment, describing what's going on here, use cases.

In particular, I'd note that the format is designed for simplicity but isn't a suitable/efficient store and isn't intended for real use by end-users.


================
Comment at: clangd/index/SymbolFromYAML.h:23
+// Convert symbols to a YAML-format string.
+std::string SymbolToYAML(const SymbolSlab& Symbols);
+
----------------
We may have multiple slabs, e.g. if we want to dump the dynamic index.
If we dump each slab and concatenate the resulting strings, is the result valid YAML (containing all symbols)?
If so, then this interface is good, but it may be worth a comment `// The YAML is safe to concatenate if you have multiple slabs`.


================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:108
+
+  std::string YAMLContent = SymbolToYAML(Symbols);
+  auto SymbolsReadFromYAML = SymbolFromYAML(YAMLContent);
----------------
round-trip is good to test the serialization, but can you add a separate test case loading a simple symbol from an actual YAML string?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41178