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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
