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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
