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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 22 00:03:38 PST 2017

sammccall added inline comments.

Comment at: clangd/CMakeLists.txt:50
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)

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list