o We only collect symbols in namespace or translation unit scopes.
o Add an option to only collect symbols in included headers.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 13628 Build 13628: arc lint + arc unit
Event Timeline
With this patch, the index-based code completion will not provide any candidates for ns1::MyClass::^ (as the index-based completion drops all results from clang's completion engine), we need a way to combine symbols from 3 sources (static index, dynamic index and clang's completion engine).
clangd/index/SymbolCollector.cpp | ||
---|---|---|
80 | These ast matchers seems to be relatively simple, maybe we can use the Decl methods to implement them at the moment. Or will we add more complex filters in the future? | |
125 | How about moving the this function inside the if below? so we don't duplicate the dyn_cast twice. | |
clangd/index/SymbolCollector.h | ||
27 | We need documentation for the options. |
Given that index-based completion is still experimental I think it's still fine to address this in a follow-up change. (In order to keep both changes simpler and more focused).
clangd/index/FileIndex.cpp | ||
---|---|---|
22–24 | NIT: why not SymbolCollector::Options CollectorOpts? It's seems more concise. Or even SymbolCollector::Options{/*IndexMainFiles=*/true}; | |
clangd/index/SymbolCollector.cpp | ||
80 | I second this. Also the code would probably be more readable without matchers in this particular example. | |
86 | ||
clangd/index/SymbolCollector.h | ||
27 | Also, would naming it OnlyMainFiles make the intent of the option clearer? | |
unittests/clangd/FileIndexTests.cpp | ||
173–174 | Why do we need to remove static int m1? | |
unittests/clangd/SymbolCollectorTests.cpp | ||
113 | Why do we remove void f()? Maybe assert that it's not in the list of symbols instead? | |
241 | Why should we change this test? |
Thanks for the reviews!
clangd/index/FileIndex.cpp | ||
---|---|---|
22–24 | I think I had too much shared/unique pointers... :P
Yeah, this is the reason I didn't use initializer. | |
clangd/index/SymbolCollector.cpp | ||
80 | Yes, I expect to borrow more matchers from include-fixer's find-all-symbols. Also, although isExpansionInMainFile seems easy to implement, I am inclined to reuse the existing code if possible. | |
clangd/index/SymbolCollector.h | ||
27 | Actually, OnlyMainFiles is the opposite. We always index header files but optionally index main files. I've added comment to clarify... | |
unittests/clangd/SymbolCollectorTests.cpp | ||
113 | I was trying to limit the scope of each test case (there is a separate test case IgnoreClassMembers). UnorderedElementsAre should be able to capture unexpected symbols. | |
241 | By default, main files are not indexed, so I put the code in header. |
LGTM
clangd/index/SymbolCollector.h | ||
---|---|---|
20–24 | We should also document what kinds of symbols we are collecting, since this patch has changed the behavior (only collect top-level symbols). | |
unittests/clangd/SymbolCollectorTests.cpp | ||
165 | nit: although the default value is false, I'd explicitly specify CollectorOpts.IndexMainFiles = false; (readers don't need to jump to another file to see the default value). |
LGTM. I'd add a comment pointing to code in include-fixer we're borrowing, though. (See the relevant comment)
clangd/index/SymbolCollector.cpp | ||
---|---|---|
80 | Could you add a comment pointing to the code in include fixer, so that everyone reading the code knows where the code comes from? | |
clangd/index/SymbolCollector.h | ||
27 | The comment looks good, thanks! | |
unittests/clangd/SymbolCollectorTests.cpp | ||
113 | Oh, sorry, I missed that the name of the test changed too. | |
241 | But we still get the symbols from main file in clangd, because we're asking to index main files in indexAST... |
- Merge branch 'master' of http://llvm.org/git/clang-tools-extra into collector
- Address review comments.
clangd/index/SymbolCollector.cpp | ||
---|---|---|
80 | Actually, I'd say we are borrowing ideas of filtering instead of the actual code from include-fixer, and the code behavior is also quite different, so it's probably not worth adding pointers to another project in the code. (But as a reference for us, the include-fixer code is .../extra/include-fixer/find-all-symbols/FindAllSymbols.cpp:119 :) |
clangd/index/SymbolCollector.cpp | ||
---|---|---|
80 | I thought we were borrowing code after reading this comment:
Thanks for the reference, I'd say it's still useful to have a reference in the code: // The code is similar to include fixer, see include-fixer/find-all-symbols/FindAllSymbols.cpp for more details. But it's up to you, of course. |
NIT: why not SymbolCollector::Options CollectorOpts? It's seems more concise.
Or even SymbolCollector::Options{/*IndexMainFiles=*/true};
The last one does not play nicely with moving the fields of the struct around, though