ilya-biryukov added inline comments.
================ Comment at: clangd/index/SymbolCollector.cpp:80 + : decl(unless(isExpansionInMainFile())), + hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())))), + *D, *ASTCtx) ---------------- ioeric wrote: > ilya-biryukov wrote: > > ioeric wrote: > > > ilya-biryukov wrote: > > > > hokein wrote: > > > > > 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? > > > > I second this. Also the code would probably be more readable without > > > > matchers in this particular example. > > > 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. > > 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) > 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 :) 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. Repository: rL LLVM https://reviews.llvm.org/D41823 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
