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

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 14 01:30:50 PST 2017

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.

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list