ilya-biryukov added a comment.
In https://reviews.llvm.org/D41823#970725, @hokein wrote:
> 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).
================
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;
----------------
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
================
Comment at: clangd/index/SymbolCollector.cpp:80
+ : decl(unless(isExpansionInMainFile())),
+ hasDeclContext(anyOf(namespaceDecl(),
translationUnitDecl())))),
+ *D, *ASTCtx)
----------------
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.
================
Comment at: clangd/index/SymbolCollector.cpp:86
+ // FIXME: Should we include the internal linkage symbols?
+ if (!ND->hasExternalFormalLinkage() || ND->isInAnonymousNamespace())
+ return true;
----------------
This should probably include changes from @hokein 's patch D41759.
================
Comment at: clangd/index/SymbolCollector.h:28
+ struct Options {
+ bool IndexMainFiles = false;
+ };
----------------
hokein wrote:
> We need documentation for the options.
Also, would naming it `OnlyMainFiles` make the intent of the option clearer?
================
Comment at: unittests/clangd/FileIndexTests.cpp:173
M.update(Ctx, "f1",
- build("f1", "class X { static int m1; int m2;};").getPointer());
+ build("f1", "class X { int m2; static void f(); };").getPointer());
----------------
Why do we need to remove `static int m1`?
Perhaps simply adding `void f()` is enough?
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:107
const std::string Header = R"(
- class Foo {
- void f();
- };
+ class Foo {};
void f1();
----------------
Why do we remove `void f()`? Maybe assert that it's not in the list of symbols
instead?
================
Comment at: unittests/clangd/SymbolCollectorTests.cpp:182
)";
- runSymbolCollector(/*Header=*/"", Main);
+ runSymbolCollector(Header, /*Main=*/"");
EXPECT_THAT(
----------------
Why should we change this test?
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
