sammccall added a comment. Thanks, this looks pretty good!
================ Comment at: clang-tools-extra/clangd/index/YAMLSerialization.cpp:36 namespace { using RefBundle = ---------------- this is independent of adding the dump command - can we split this into a separate patch? Partly because it will need to have some basic testing :-( ================ Comment at: clang-tools-extra/clangd/index/YAMLSerialization.cpp:355 + + FileDigest denormalize(IO &I) { + auto Digest = fromStr(HexString); ---------------- this + 2 helpers seems a bit verbose/indirect. ``` FileDigest D; if (HexString.size() == D.size() && llvm::all_of(HexString, llvm:isHexDigit)) { memcpy(Digest.data(), llvm::fromHex(HexString).data(), Digest.size()); } else { I.setError("Bad hex file digest"); } return D; ``` ================ Comment at: clang-tools-extra/clangd/index/YAMLSerialization.cpp:380 + +template <> struct MappingTraits<CompileCommand> { + static void mapping(IO &IO, CompileCommand &Cmd) { ---------------- Unfortunately we don't own the CompileCommand type, so we shouldn't specialize traits for it (risk ODR violation if someone else does the same). Wrap or inherit it in a trivial struct `struct CompileCommandYAML : tooling::CompileCommand {}` to make this safe. ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:288 + IndexOut.Format = Format; + llvm::outs() << IndexOut; + return 0; ---------------- Writing it to stdout doesn't seem unreasonable but maybe not the right default. What about requiring a filename arg, and creating a `raw_fd_ostream` - then you can pass `-` to get stdout. ================ Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:322 + // Make Export command option(s) available on command line. + // That allows for convenient (piping/redirecting) a dump non-interactively + // without passing through REPL. ---------------- I wonder if we should generalize this instead to running an arbitrary command non-interactively: `dexp -c "dump"` No need to do that in this patch but maybe leave a TODO CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77385/new/ https://reviews.llvm.org/D77385 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits