This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add more filters for collected symbols.
ClosedPublic

Authored by ioeric on Jan 8 2018, 8:34 AM.

Details

Summary

o We only collect symbols in namespace or translation unit scopes.
o Add an option to only collect symbols in included headers.

Event Timeline

ioeric created this revision.Jan 8 2018, 8:34 AM
hokein added a comment.Jan 9 2018, 1:48 AM

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.

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).

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};
The last one does not play nicely with moving the fields of the struct around, though

clangd/index/SymbolCollector.cpp
80

I second this. Also the code would probably be more readable without matchers in this particular example.

86

This should probably include changes from @hokein 's patch D41759.

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?
Perhaps simply adding void f() is enough?

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?

ioeric updated this revision to Diff 129078.Jan 9 2018, 7:08 AM
ioeric marked 4 inline comments as done.
  • Addressed comments.
  • Merged with origin/master
  • Merge with diff base.
ioeric added a comment.Jan 9 2018, 7:10 AM

Thanks for the reviews!

clangd/index/FileIndex.cpp
22–24

I think I had too much shared/unique pointers... :P

The last one does not play nicely with moving the fields of the struct around, though

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.

ioeric updated this revision to Diff 129112.Jan 9 2018, 9:40 AM
  • Rebase on origin/master
hokein accepted this revision.Jan 10 2018, 5:22 AM

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).

This revision is now accepted and ready to land.Jan 10 2018, 5:22 AM
ilya-biryukov accepted this revision.Jan 10 2018, 6:02 AM

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?
(It's generally useful in case there are bugs and borrowing more code from include-fixer fixes that)

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.
IMO, generally, adding new tests in seems like a better option if the original tests aren't broken.

241

But we still get the symbols from main file in clangd, because we're asking to index main files in indexAST...
Makes sense. Thanks for clarifying.

ioeric updated this revision to Diff 129275.Jan 10 2018, 6:54 AM
ioeric marked 2 inline comments as done.
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 :)

This revision was automatically updated to reflect the committed changes.
ilya-biryukov added inline comments.Jan 10 2018, 8:05 AM
clangd/index/SymbolCollector.cpp
80

I thought we were borrowing code after reading this comment:

I expect to borrow more matchers from include-fixer's find-all-symbols.

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.