[PATCH] D41491: [clangd] Add a tool to build YAML-format global symbols.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/CMakeLists.txt:50
 add_subdirectory(tool)
+add_subdirectory(global-symbol-builder)
----------------
I think generally we run `check-clang-tools` before committing - I guess it's OK not to have tests for this experimental tool, but we should at least keep it building. Can you make check-clang-tools build it?


================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:106
+  std::error_code EC;
+  SymbolSlab Result;
+  std::mutex SymbolMutex;
----------------
FYI D41506 changes this API, adding `SymbolSlab::Builder`. Shouldn't be a problem, but we'll need to merge.


================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:112
+    for (const auto &Symbol : NewSymbols) {
+      auto it = Result.find(Symbol.second.ID);
+      if (it == Result.end())
----------------
This seems like you might end up overwriting good declarations with bad ones (e.g. forward decls). Picking one with a simple heuristic like "longest range" would get classes right.


================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:175
+          *Executor->get()->getExecutionContext()));
+  auto Err = Executor->get()->execute(std::move(T));
+  if (Err) {
----------------
I don't understand why we need to plumb callbacks through SymbolCollector.
Can't we just write the slab after this line?


================
Comment at: clangd/global-symbol-builder/run-global-symbol-builder.py:1
+#!/usr/bin/env python
+#
----------------
So we now have three copies of this script...
I'm probably missing something, but what's here that's hard to do in a single binary?
I'd have to think it'd be faster if we weren't constantly spawning new processes and roundtripping all the data through YAML-on-disk.

(If this is just expedience because we have an existing template to copy, I'm happy to try to combine it into one binary myself)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41491