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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comments.



================
Comment at: clangd/CMakeLists.txt:50
 add_subdirectory(tool)
+add_subdirectory(global-symbol-builder)
----------------
sammccall wrote:
> 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?
Yeah, it builds.


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


================
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())
----------------
sammccall wrote:
> 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.
Add a FIXME here, will do it in a follow-up -- as the symbol structure would be changed.


================
Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:175
+          *Executor->get()->getExecutionContext()));
+  auto Err = Executor->get()->execute(std::move(T));
+  if (Err) {
----------------
sammccall wrote:
> I don't understand why we need to plumb callbacks through SymbolCollector.
> Can't we just write the slab after this line?
`ToolExecutor` takes over the lifetime of `FrontendActionFactory`, there is no straightforward way to get the internal `SymbolSlab` from the indexAction.

Changed to use `ClangTool`, since it would simplify the code a lot, and no changes in `SymbolCollector`.


================
Comment at: clangd/global-symbol-builder/run-global-symbol-builder.py:1
+#!/usr/bin/env python
+#
----------------
sammccall wrote:
> 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)
Yeah, the script is borrowed from other places for the fast prototype.

We can combine it into the `global-symbol-builder` binary. Done.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41491