ioeric added a comment.
Thanks for the reviews!
================
Comment at: clangd/index/FileIndex.cpp:22
llvm::ArrayRef<const Decl *> Decls) {
- auto Collector = std::make_shared<SymbolCollector>();
+ auto CollectorOpts = SymbolCollector::Options();
+ CollectorOpts.IndexMainFiles = true;
----------------
ilya-biryukov wrote:
> 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
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.
================
Comment at: clangd/index/SymbolCollector.cpp:80
+ : decl(unless(isExpansionInMainFile())),
+ hasDeclContext(anyOf(namespaceDecl(),
translationUnitDecl())))),
+ *D, *ASTCtx)
----------------
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.
================
Comment at: clangd/index/SymbolCollector.h:28
+ struct Options {
+ bool IndexMainFiles = false;
+ };
----------------
ilya-biryukov wrote:
> hokein wrote:
> > We need documentation for the options.
> Also, would naming it `OnlyMainFiles` make the intent of the option clearer?
Actually, `OnlyMainFiles` is the opposite. We always index header files but
optionally index main files. I've added comment to clarify...
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:107
const std::string Header = R"(
- class Foo {
- void f();
- };
+ class Foo {};
void f1();
----------------
ilya-biryukov wrote:
> Why do we remove `void f()`? Maybe assert that it's not in the list of
> symbols instead?
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.
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:182
)";
- runSymbolCollector(/*Header=*/"", Main);
+ runSymbolCollector(Header, /*Main=*/"");
EXPECT_THAT(
----------------
ilya-biryukov wrote:
> Why should we change this test?
By default, main files are not indexed, so I put the code in header.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41823
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
