[PATCH] D77385: [clangd] Add index export to dexp

[Sam McCall via Phabricator via cfe-commits]

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