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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments!



================
Comment at: clangd/index/SymbolFromYAML.cpp:32
+// supported in YAML I/O.
+struct NPArray {
+  NPArray(IO &) {}
----------------
sammccall wrote:
> what does NP stand for?
Ah, P is a typo here. N stands for Normalized. Renamed it to `NormalizedXXX`.


================
Comment at: clangd/index/SymbolFromYAML.cpp:44
+
+  std::vector<uint8_t> Value;
+};
----------------
sammccall wrote:
> 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.
Good idea. Done.


================
Comment at: clangd/index/SymbolFromYAML.h:1
+//===--- SymbolFromYAML.h ----------------------------------------*- C++-*-===//
+//
----------------
sammccall wrote:
> nit: `SymbolFromYAML.h` seems slightly off for the file, because you support both conversions (and also multiple symbols)
> 
> Consider just `YAML.h`?
`YAML` seems too general. Named it `SymbolYAML` indicating the conversion between Symbol and YAML.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41178