[PATCH] D44954: [clangd] Add "member" symbols to the index
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334017: [clangd] Add "member" symbols to the index
(authored by malaperle, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D44954
Files:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/index/Index.h
clang-tools-extra/trunk/clangd/index/MemIndex.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -32,6 +32,7 @@
using ::testing::Each;
using ::testing::ElementsAre;
using ::testing::Field;
+using ::testing::IsEmpty;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
@@ -153,6 +154,7 @@
Sym.CompletionSnippetInsertText = Sym.Name;
Sym.CompletionLabel = Sym.Name;
Sym.SymInfo.Kind = Kind;
+ Sym.IsIndexedForCodeCompletion = true;
return Sym;
}
Symbol func(StringRef Name) { // Assumes the function has no args.
@@ -684,6 +686,20 @@
Contains(AllOf(Named("baz"), Doc("Multi-line\nblock comment";
}
+TEST(CompletionTest, GlobalCompletionFiltering) {
+
+ Symbol Class = cls("XYZ");
+ Class.IsIndexedForCodeCompletion = false;
+ Symbol Func = func("XYZ::f");
+ Func.IsIndexedForCodeCompletion = false;
+
+ auto Results = completions(R"(// void f() {
+ XYZ::f^
+ })",
+ {Class, Func});
+ EXPECT_THAT(Results.items, IsEmpty());
+}
+
TEST(CodeCompleteTest, DisableTypoCorrection) {
auto Results = completions(R"cpp(
namespace clang { int v; }
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
Pos.end.character);
}
MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
+ return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
+}
namespace clang {
namespace clangd {
@@ -132,9 +135,13 @@
CollectorOpts, PragmaHandler.get());
std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName, TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11", "-include", TestHeaderName};
Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+// This allows to override the "-xc++" with something else, i.e.
+// -xobjective-c++.
+Args.push_back(TestFileName);
+
tooling::ToolInvocation Invocation(
Args,
Factory->create(), Files.get(),
@@ -163,8 +170,20 @@
TEST_F(SymbolCollectorTest, CollectSymbols) {
const std::string Header = R"(
class Foo {
+ Foo() {}
+ Foo(int a) {}
void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
+ void f();
+ };
};
+class Friend {
+};
+
void f1();
inline void f2() {}
static const int KInt = 2;
@@ -200,23 +219,78 @@
runSymbolCollector(Header, /*Main=*/"");
EXPECT_THAT(Symbols,
UnorderedElementsAreArray(
- {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
- QName("kStr"), QName("foo"), QName("foo::bar"),
- QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
- QName("foo::bar::v2"), QName("foo::baz")}));
+ {AllOf(QName("Foo"), ForCodeCompletion(true)),
+ AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::f"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
+
+ AllOf(QName("Friend"), ForCodeCompletion(true)),
+
[PATCH] D44954: [clangd] Add "member" symbols to the index
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE334017: [clangd] Add "member" symbols to the
index (authored by malaperle, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D44954?vs=149532&id=149970#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -18,13 +18,18 @@
namespace clang {
namespace clangd {
-/// \brief Collect top-level symbols from an AST. These are symbols defined
-/// immediately inside a namespace or a translation unit scope. For example,
-/// symbols in classes or functions are not collected. Note that this only
-/// collects symbols that declared in at least one file that is not a main
-/// file (i.e. the source file corresponding to a TU). These are symbols that
-/// can be imported by other files by including the file where symbols are
-/// declared.
+/// \brief Collect declarations (symbols) from an AST.
+/// It collects most declarations except:
+/// - Implicit declarations
+/// - Anonymous declarations (anonymous enum/class/struct, etc)
+/// - Declarations in anonymous namespaces
+/// - Local declarations (in function bodies, blocks, etc)
+/// - Declarations in main files
+/// - Template specializations
+/// - Library-specific private declarations (e.g. private declaration generated
+/// by protobuf compiler)
+///
+/// See also shouldFilterDecl().
///
/// Clients (e.g. clangd) can use SymbolCollector together with
/// index::indexTopLevelDecls to retrieve all symbols when the source file is
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -9,6 +9,7 @@
#include "SymbolCollector.h"
#include "../AST.h"
+#include "../CodeComplete.h"
#include "../CodeCompletionStrings.h"
#include "../Logger.h"
#include "../SourceCode.h"
@@ -149,21 +150,20 @@
if (ND->isInAnonymousNamespace())
return true;
- // We only want:
- // * symbols in namespaces or translation unit scopes (e.g. no class
- // members)
- // * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
- auto InTopLevelScope = hasDeclContext(
- anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
- // Don't index template specializations.
+ // We want most things but not "local" symbols such as symbols inside
+ // FunctionDecl, BlockDecl, ObjCMethodDecl and OMPDeclareReductionDecl.
+ // FIXME: Need a matcher for ExportDecl in order to include symbols declared
+ // within an export.
+ auto InNonLocalContext = hasDeclContext(anyOf(
+ translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+ enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+ objcCategoryImplDecl(), objcImplementationDecl()));
+ // Don't index template specializations and expansions in main files.
auto IsSpecialization =
anyOf(functionDecl(isExplicitTemplateSpecialization()),
cxxRecordDecl(isExplicitTemplateSpecialization()),
varDecl(isExplicitTemplateSpecialization()));
- if (match(decl(allOf(unless(isExpansionInMainFile()),
- anyOf(InTopLevelScope,
- hasDeclContext(enumDecl(InTopLevelScope,
- unless(isScoped(),
+ if (match(decl(allOf(unless(isExpansionInMainFile()), InNonLocalContext,
unless(IsSpecialization))),
*ND, *ASTCtx)
.empty())
@@ -377,6 +377,8 @@
Symbol S;
S.ID = std::move(ID);
std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
+
+ S.IsIndexedForCodeCompletion = isIndexedForCodeCompletion(ND, Ctx);
S.SymInfo = index::getSymbolInfo(&ND);
std::string FileURI;
if (auto DeclLoc =
Index: clangd/index/Index.h
===
--- clangd/index/Index.h
+++ clangd/index/Index.h
@@ -149,9 +149,11 @@
// The number of translation units that reference this symbol from their main
// file. This number is only meaningful if aggregated in an index.
unsigned References = 0;
-
+ /// Whether or not this symbol is meant to be used for the code completion.
+ /// See also isIndexedForCodeCompletion().
+ bool IsIndexedForCodeCompletion = false;
/// A brief description of the symbol that can be displayed in the completion
-
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle updated this revision to Diff 149532.
malaperle marked 4 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
Pos.end.character);
}
MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
+ return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
+}
namespace clang {
namespace clangd {
@@ -132,9 +135,13 @@
CollectorOpts, PragmaHandler.get());
std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName, TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11", "-include", TestHeaderName};
Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+// This allows to override the "-xc++" with something else, i.e.
+// -xobjective-c++.
+Args.push_back(TestFileName);
+
tooling::ToolInvocation Invocation(
Args,
Factory->create(), Files.get(),
@@ -163,8 +170,20 @@
TEST_F(SymbolCollectorTest, CollectSymbols) {
const std::string Header = R"(
class Foo {
+ Foo() {}
+ Foo(int a) {}
void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
+ void f();
+ };
+};
+class Friend {
};
+
void f1();
inline void f2() {}
static const int KInt = 2;
@@ -200,23 +219,78 @@
runSymbolCollector(Header, /*Main=*/"");
EXPECT_THAT(Symbols,
UnorderedElementsAreArray(
- {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
- QName("kStr"), QName("foo"), QName("foo::bar"),
- QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
- QName("foo::bar::v2"), QName("foo::baz")}));
+ {AllOf(QName("Foo"), ForCodeCompletion(true)),
+ AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::f"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
+
+ AllOf(QName("Friend"), ForCodeCompletion(true)),
+ AllOf(QName("f1"), ForCodeCompletion(true)),
+ AllOf(QName("f2"), ForCodeCompletion(true)),
+ AllOf(QName("KInt"), ForCodeCompletion(true)),
+ AllOf(QName("kStr"), ForCodeCompletion(true)),
+ AllOf(QName("foo"), ForCodeCompletion(true)),
+ AllOf(QName("foo::bar"), ForCodeCompletion(true)),
+ AllOf(QName("foo::int32"), ForCodeCompletion(true)),
+ AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
+ AllOf(QName("foo::v1"), ForCodeCompletion(true)),
+ AllOf(QName("foo::bar::v2"), ForCodeCompletion(true)),
+ AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
}
TEST_F(SymbolCollectorTest, Template) {
Annotations Header(R"(
// Template is indexed, specialization and instantiation is not.
-template struct [[Tmpl]] {T x = 0;};
+template struct [[Tmpl]] {T $xdecl[[x]] = 0;};
template <> struct Tmpl {};
extern template struct Tmpl;
template struct Tmpl;
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
- QName("Tmpl"), DeclRange(Header.range()))}));
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAreArray(
+ {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+ AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+ const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added inline comments.
Comment at: unittests/clangd/CodeCompleteTests.cpp:741
+TEST(CompletionTest, Enums) {
+ EXPECT_THAT(completions(R"cpp(
ioeric wrote:
> malaperle wrote:
> > ioeric wrote:
> > > It's not clear to me what the following tests (`Enums`,
> > > `AnonymousNamespace`, `InMainFile`) are testing. Do they test code
> > > completion or symbol collector? If these test symbol collection, they
> > > should be moved int SymbolCollectorTest.cpp
> > They are testing that code completion works as intended regardless of how
> > symbol collector is implemented. It's similar to our previous discussion in
> > D44882 about "black box testing". I can remove them but it seems odd to me
> > to not have code completion level tests for all cases because we assume
> > that it will behave a certain way at the symbol collection and querying
> > levels.
> FWIW, I am not against those tests at all; increasing coverage is always a
> nice thing to do IMO. I just thought it would make more sense to add them in
> a separate patch if they are not related to changes in this patch; I found
> unrelated tests a bit confusing otherwise.
> I found unrelated tests a bit confusing otherwise.
Fair point!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
sammccall accepted this revision. sammccall added a comment. Thanks, LG! Comment at: clangd/CodeComplete.h:86 +// For index-based completion, we only want: +// * symbols in namespaces or translation unit scopes (e.g. no class nit: want -> consider? Comment at: clangd/CodeComplete.h:94 +// lookup rules. +bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx); } // namespace clangd A little more context: "// Other symbols still appear in the index for other purposes, like workspace/symbols or textDocument/definition, but are not used for code completion" Comment at: clangd/index/SymbolCollector.h:22 +/// \brief Collect symbols from an AST. +/// It collects most symbols except: +/// - Implicit symbols nit: can you change "symbols" here to "declarations"? currently we rely a bit too heavily on the user knowing what "symbol" means Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
lgtm
Comment at: clangd/index/Index.h:158
unsigned References = 0;
-
+ /// Whether or not this is symbol is meant to be used for the global
+ /// completion.
malaperle wrote:
> sammccall wrote:
> > ioeric wrote:
> > > s/this is symbol/this symbol/?
> > Sorry, I hadn't seen this patch until now.
> > When it was part of the `workspace/symbol` patch, I was the other person
> > concerned this was too coupled to existing use cases and preferred
> > something more descriptive.
> >
> > I dug up an analysis @hokein and I worked on. One concluseion we came to
> > was that we thought results needed by `completion` were a subset of what
> > `workspace/symbol` needs, so a boolean would work. It seemed a bit ad-hoc
> > and fragile though.
> >
> > The cases we looked at were:
> >
> > ||private|member|local|primary template|template specialization|nested in
> > template|
> > | code complete |N|N|N|Y|N|N|
> > | workspace/symbol |Y|Y|N|Y|Y|Y|
> > | go to defn |Y|Y|?|?|?|?|
> > (Y = index should return it, N = index should not return it, ? = don't care)
> >
> > So the most relevant information seems to be:
> > - is this private (private member, internal linkage, no decl outside cpp
> > file, etc)
> > - is this nested in a class type (or template)
> > - is this a template specialization
> >
> > I could live with bundling these into a single property (though they seem
> > like good ranking signals, and we'd lose them for that purpose), it will
> > certainly make a posting-list based index more efficient.
> >
> > In that case I think we should have canonical documentation *somewhere*
> > about exactly what this subset is, and this field should reference that.
> > e.g. `isIndexedForCodeCompletion()` in `CodeComplete.h` with docs and
> > `IsIndexedForCodeCompletion` here. (Please avoid "global" as it has too
> > many different meanings - here we mean "index-based").
> OK, I added documentation on the SymbolCollector (which was outdated) about
> what kinds of symbols are collected, with a reference to shouldFilterDecl.
> The subset is documented on isIndexedForCodeCompletion(), also referenced
> from the Symbol field. Was that more or less what you meant?
> I could live with bundling these into a single property (though they seem
> like good ranking signals, and we'd lose them for that purpose), it will
> certainly make a posting-list based index more efficient.
FWIW, I think we could still add those signals when we need them in the future.
It seems reasonable to me for the code completion flag to co-exist with ranking
signals that could potentially overlap.
Comment at: unittests/clangd/CodeCompleteTests.cpp:741
+TEST(CompletionTest, Enums) {
+ EXPECT_THAT(completions(R"cpp(
malaperle wrote:
> ioeric wrote:
> > It's not clear to me what the following tests (`Enums`,
> > `AnonymousNamespace`, `InMainFile`) are testing. Do they test code
> > completion or symbol collector? If these test symbol collection, they
> > should be moved int SymbolCollectorTest.cpp
> They are testing that code completion works as intended regardless of how
> symbol collector is implemented. It's similar to our previous discussion in
> D44882 about "black box testing". I can remove them but it seems odd to me to
> not have code completion level tests for all cases because we assume that it
> will behave a certain way at the symbol collection and querying levels.
FWIW, I am not against those tests at all; increasing coverage is always a nice
thing to do IMO. I just thought it would make more sense to add them in a
separate patch if they are not related to changes in this patch; I found
unrelated tests a bit confusing otherwise.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:141
Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(TestFileName);
+
malaperle wrote:
> This allows to override the "-xc++" with something else, i.e. -xobjective-c++
I think this could also be a comment in the code :)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle updated this revision to Diff 149206.
malaperle added a comment.
Update with suggestions.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
Pos.end.character);
}
MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(ForCodeCompletion, IsIndexedForCodeCompletion, "") {
+ return arg.IsIndexedForCodeCompletion == IsIndexedForCodeCompletion;
+}
namespace clang {
namespace clangd {
@@ -132,9 +135,11 @@
CollectorOpts, PragmaHandler.get());
std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName, TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11", "-include", TestHeaderName};
Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(TestFileName);
+
tooling::ToolInvocation Invocation(
Args,
Factory->create(), Files.get(),
@@ -163,8 +168,20 @@
TEST_F(SymbolCollectorTest, CollectSymbols) {
const std::string Header = R"(
class Foo {
+ Foo() {}
+ Foo(int a) {}
void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
+ void f();
+ };
+};
+class Friend {
};
+
void f1();
inline void f2() {}
static const int KInt = 2;
@@ -200,23 +217,78 @@
runSymbolCollector(Header, /*Main=*/"");
EXPECT_THAT(Symbols,
UnorderedElementsAreArray(
- {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
- QName("kStr"), QName("foo"), QName("foo::bar"),
- QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
- QName("foo::bar::v2"), QName("foo::baz")}));
+ {AllOf(QName("Foo"), ForCodeCompletion(true)),
+ AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::f"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::~Foo"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::operator="), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Nested"), ForCodeCompletion(false)),
+ AllOf(QName("Foo::Nested::f"), ForCodeCompletion(false)),
+
+ AllOf(QName("Friend"), ForCodeCompletion(true)),
+ AllOf(QName("f1"), ForCodeCompletion(true)),
+ AllOf(QName("f2"), ForCodeCompletion(true)),
+ AllOf(QName("KInt"), ForCodeCompletion(true)),
+ AllOf(QName("kStr"), ForCodeCompletion(true)),
+ AllOf(QName("foo"), ForCodeCompletion(true)),
+ AllOf(QName("foo::bar"), ForCodeCompletion(true)),
+ AllOf(QName("foo::int32"), ForCodeCompletion(true)),
+ AllOf(QName("foo::int32_t"), ForCodeCompletion(true)),
+ AllOf(QName("foo::v1"), ForCodeCompletion(true)),
+ AllOf(QName("foo::bar::v2"), ForCodeCompletion(true)),
+ AllOf(QName("foo::baz"), ForCodeCompletion(true))}));
}
TEST_F(SymbolCollectorTest, Template) {
Annotations Header(R"(
// Template is indexed, specialization and instantiation is not.
-template struct [[Tmpl]] {T x = 0;};
+template struct [[Tmpl]] {T $xdecl[[x]] = 0;};
template <> struct Tmpl {};
extern template struct Tmpl;
template struct Tmpl;
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
- QName("Tmpl"), DeclRange(Header.range()))}));
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAreArray(
+ {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+ AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+ const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName{
+ int foo;
+ ^(int param){ int bar; };
+}
+@end
+
+@interface Person (MyCategory)
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle marked an inline comment as done. malaperle added inline comments. Comment at: clangd/index/Index.h:158 unsigned References = 0; - + /// Whether or not this is symbol is meant to be used for the global + /// completion. sammccall wrote: > ioeric wrote: > > s/this is symbol/this symbol/? > Sorry, I hadn't seen this patch until now. > When it was part of the `workspace/symbol` patch, I was the other person > concerned this was too coupled to existing use cases and preferred something > more descriptive. > > I dug up an analysis @hokein and I worked on. One concluseion we came to was > that we thought results needed by `completion` were a subset of what > `workspace/symbol` needs, so a boolean would work. It seemed a bit ad-hoc and > fragile though. > > The cases we looked at were: > > ||private|member|local|primary template|template specialization|nested in > template| > | code complete |N|N|N|Y|N|N| > | workspace/symbol |Y|Y|N|Y|Y|Y| > | go to defn |Y|Y|?|?|?|?| > (Y = index should return it, N = index should not return it, ? = don't care) > > So the most relevant information seems to be: > - is this private (private member, internal linkage, no decl outside cpp > file, etc) > - is this nested in a class type (or template) > - is this a template specialization > > I could live with bundling these into a single property (though they seem > like good ranking signals, and we'd lose them for that purpose), it will > certainly make a posting-list based index more efficient. > > In that case I think we should have canonical documentation *somewhere* about > exactly what this subset is, and this field should reference that. > e.g. `isIndexedForCodeCompletion()` in `CodeComplete.h` with docs and > `IsIndexedForCodeCompletion` here. (Please avoid "global" as it has too many > different meanings - here we mean "index-based"). OK, I added documentation on the SymbolCollector (which was outdated) about what kinds of symbols are collected, with a reference to shouldFilterDecl. The subset is documented on isIndexedForCodeCompletion(), also referenced from the Symbol field. Was that more or less what you meant? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
sammccall added a subscriber: hokein. sammccall added inline comments. Comment at: clangd/index/Index.h:158 unsigned References = 0; - + /// Whether or not this is symbol is meant to be used for the global + /// completion. ioeric wrote: > s/this is symbol/this symbol/? Sorry, I hadn't seen this patch until now. When it was part of the `workspace/symbol` patch, I was the other person concerned this was too coupled to existing use cases and preferred something more descriptive. I dug up an analysis @hokein and I worked on. One concluseion we came to was that we thought results needed by `completion` were a subset of what `workspace/symbol` needs, so a boolean would work. It seemed a bit ad-hoc and fragile though. The cases we looked at were: ||private|member|local|primary template|template specialization|nested in template| | code complete |N|N|N|Y|N|N| | workspace/symbol |Y|Y|N|Y|Y|Y| | go to defn |Y|Y|?|?|?|?| (Y = index should return it, N = index should not return it, ? = don't care) So the most relevant information seems to be: - is this private (private member, internal linkage, no decl outside cpp file, etc) - is this nested in a class type (or template) - is this a template specialization I could live with bundling these into a single property (though they seem like good ranking signals, and we'd lose them for that purpose), it will certainly make a posting-list based index more efficient. In that case I think we should have canonical documentation *somewhere* about exactly what this subset is, and this field should reference that. e.g. `isIndexedForCodeCompletion()` in `CodeComplete.h` with docs and `IsIndexedForCodeCompletion` here. (Please avoid "global" as it has too many different meanings - here we mean "index-based"). Comment at: clangd/index/Index.h:279 + /// considered. + bool GlobalCodeCompletionOnly = false; }; please also avoid "global" here, e.g. `RestrictForCodeCompletion` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added inline comments. Comment at: unittests/clangd/SymbolCollectorTests.cpp:141 Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end()); +Args.push_back(TestFileName); + This allows to override the "-xc++" with something else, i.e. -xobjective-c++ Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added inline comments.
Comment at: clangd/index/SymbolCollector.cpp:158
+ translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+ enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+ objcCategoryImplDecl(), objcImplementationDecl()));
ioeric wrote:
> (Disclaimer: I don't know obj-c...)
>
> It seems that some objc contexts here are good for global code completion. If
> we want to support objc symbols, it might also make sense to properly set
> `SupportGlobalCompletion` for them.
Sounds good. Maybe it would be OK to do this in another (small) patch? I also
know next to nothing about obj-c :)
Comment at: unittests/clangd/CodeCompleteTests.cpp:741
+TEST(CompletionTest, Enums) {
+ EXPECT_THAT(completions(R"cpp(
ioeric wrote:
> It's not clear to me what the following tests (`Enums`, `AnonymousNamespace`,
> `InMainFile`) are testing. Do they test code completion or symbol collector?
> If these test symbol collection, they should be moved int
> SymbolCollectorTest.cpp
They are testing that code completion works as intended regardless of how
symbol collector is implemented. It's similar to our previous discussion in
D44882 about "black box testing". I can remove them but it seems odd to me to
not have code completion level tests for all cases because we assume that it
will behave a certain way at the symbol collection and querying levels.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle updated this revision to Diff 148834.
malaperle marked 6 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
Files:
clangd/CodeComplete.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
Pos.end.character);
}
MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(Global, SupportGlobalCompletion, "") {
+ return arg.SupportGlobalCompletion == SupportGlobalCompletion;
+}
namespace clang {
namespace clangd {
@@ -132,9 +135,11 @@
CollectorOpts, PragmaHandler.get());
std::vector Args = {
-"symbol_collector", "-fsyntax-only", "-xc++", "-std=c++11",
-"-include", TestHeaderName, TestFileName};
+"symbol_collector", "-fsyntax-only", "-xc++",
+"-std=c++11", "-include", TestHeaderName};
Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(TestFileName);
+
tooling::ToolInvocation Invocation(
Args,
Factory->create(), Files.get(),
@@ -163,8 +168,20 @@
TEST_F(SymbolCollectorTest, CollectSymbols) {
const std::string Header = R"(
class Foo {
+ Foo() {}
+ Foo(int a) {}
void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
+ void f();
+ };
+};
+class Friend {
};
+
void f1();
inline void f2() {}
static const int KInt = 2;
@@ -198,25 +215,79 @@
} // namespace foo
)";
runSymbolCollector(Header, /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAreArray(
- {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
- QName("kStr"), QName("foo"), QName("foo::bar"),
- QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
- QName("foo::bar::v2"), QName("foo::baz")}));
+ EXPECT_THAT(Symbols, UnorderedElementsAreArray(
+ {AllOf(QName("Foo"), Global(true)),
+AllOf(QName("Foo::Foo"), Global(false)),
+AllOf(QName("Foo::Foo"), Global(false)),
+AllOf(QName("Foo::f"), Global(false)),
+AllOf(QName("Foo::~Foo"), Global(false)),
+AllOf(QName("Foo::operator="), Global(false)),
+AllOf(QName("Foo::Nested"), Global(false)),
+AllOf(QName("Foo::Nested::f"), Global(false)),
+
+AllOf(QName("Friend"), Global(true)),
+AllOf(QName("f1"), Global(true)),
+AllOf(QName("f2"), Global(true)),
+AllOf(QName("KInt"), Global(true)),
+AllOf(QName("kStr"), Global(true)),
+AllOf(QName("foo"), Global(true)),
+AllOf(QName("foo::bar"), Global(true)),
+AllOf(QName("foo::int32"), Global(true)),
+AllOf(QName("foo::int32_t"), Global(true)),
+AllOf(QName("foo::v1"), Global(true)),
+AllOf(QName("foo::bar::v2"), Global(true)),
+AllOf(QName("foo::baz"), Global(true))}));
}
TEST_F(SymbolCollectorTest, Template) {
Annotations Header(R"(
// Template is indexed, specialization and instantiation is not.
-template struct [[Tmpl]] {T x = 0;};
+template struct [[Tmpl]] {T $xdecl[[x]] = 0;};
template <> struct Tmpl {};
extern template struct Tmpl;
template struct Tmpl;
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
- QName("Tmpl"), DeclRange(Header.range()))}));
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAreArray(
+ {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+ AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+ const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName{
+ int foo;
+ ^(int param){ int bar; };
+}
+@end
+
+@interface Person (MyCategor
[PATCH] D44954: [clangd] Add "member" symbols to the index
ioeric added a comment.
The change looks mostly good. Some nits and questions about the testing.
Comment at: clangd/index/Index.h:158
unsigned References = 0;
-
+ /// Whether or not this is symbol is meant to be used for the global
+ /// completion.
s/this is symbol/this symbol/?
Comment at: clangd/index/SymbolCollector.cpp:158
+ translationUnitDecl(), namespaceDecl(), linkageSpecDecl(), recordDecl(),
+ enumDecl(), objcProtocolDecl(), objcInterfaceDecl(), objcCategoryDecl(),
+ objcCategoryImplDecl(), objcImplementationDecl()));
(Disclaimer: I don't know obj-c...)
It seems that some objc contexts here are good for global code completion. If
we want to support objc symbols, it might also make sense to properly set
`SupportGlobalCompletion` for them.
Comment at: clangd/index/SymbolCollector.cpp:359
+
+ // For global completion, we only want:
+ // * symbols in namespaces or translation unit scopes (e.g. no class
Could we pull this into a helper function? Something like
`S.SupportGlobalCompletion = DeclSupportsGlobalCompletion(...)`?
Comment at: unittests/clangd/CodeCompleteTests.cpp:657
+TEST(CompletionTest, DynamicIndexMultiFileMembers) {
+ MockFSProvider FS;
IIUC, this tests the `SupportGlobalCompletion` filtering logic for index
completion? If so, I think it would be more straightforward to do something
like:
```
Sym NoGlobal(...);
NoGlobal.SupportGlobalCompletion = false;
...
completions (Code, {NoGlobal, func(...), cls(...)})
// Expect only global symbols in completion results.
```
This avoid setting up the ClangdServer manually.
Comment at: unittests/clangd/CodeCompleteTests.cpp:741
+TEST(CompletionTest, Enums) {
+ EXPECT_THAT(completions(R"cpp(
It's not clear to me what the following tests (`Enums`, `AnonymousNamespace`,
`InMainFile`) are testing. Do they test code completion or symbol collector?
If these test symbol collection, they should be moved int
SymbolCollectorTest.cpp
Comment at: unittests/clangd/CodeCompleteTests.cpp:785
+ .items,
+ IsEmpty());
+
I think the behaviors above are the same before this change, so I'm not quite
sure what they are testing. Note that symbols in main files are not collected
into dynamic index, and by default the tests do not use dynamic index.
Comment at: unittests/clangd/CodeCompleteTests.cpp:846
+ auto Results = cantFail(runCodeComplete(Server, File, Test.point(), {}));
+ EXPECT_THAT(Results.items, IsEmpty());
+}
I think this is expected to be empty even for symbols that are not in anonymous
namespace because symbols in main files are not indexed.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:70
MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(SupportGlobalCompletion, SupportGlobalCompletion, "") {
+ return arg.SupportGlobalCompletion == SupportGlobalCompletion;
nit: I'd probably call this `Global` for convenience.
Comment at: unittests/clangd/SymbolCollectorTests.cpp:216
runSymbolCollector(Header, /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAreArray(
- {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
- QName("kStr"), QName("foo"), QName("foo::bar"),
- QName("foo::int32"), QName("foo::int32_t"),
QName("foo::v1"),
- QName("foo::bar::v2"), QName("foo::baz")}));
+ EXPECT_THAT(Symbols, UnorderedElementsAreArray(
+ {QName("Foo"), QName("Foo::Foo"),
Could you also add checkers for the `SupportGlobalCompletion` field?
Comment at: unittests/clangd/SymbolCollectorTests.cpp:404
runSymbolCollector(Header, /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-QName("Green"), QName("Color2"),
-QName("ns"), QName("ns::Black")));
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(QName("Red"), QName("Color"),
QName("Green"),
Could you please also add checkers for `GlobalCodeCompletion` for these enums?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle updated this revision to Diff 148329.
malaperle added a comment.
Use "SupportGlobalCompletion", white-list decl contexts, add more tests
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
Files:
clangd/CodeComplete.cpp
clangd/index/Index.h
clangd/index/MemIndex.cpp
clangd/index/SymbolCollector.cpp
clangd/index/SymbolYAML.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -67,6 +67,9 @@
Pos.end.character);
}
MATCHER_P(Refs, R, "") { return int(arg.References) == R; }
+MATCHER_P(SupportGlobalCompletion, SupportGlobalCompletion, "") {
+ return arg.SupportGlobalCompletion == SupportGlobalCompletion;
+}
namespace clang {
namespace clangd {
@@ -163,8 +166,20 @@
TEST_F(SymbolCollectorTest, CollectSymbols) {
const std::string Header = R"(
class Foo {
+ Foo() {}
+ Foo(int a) {}
+ void f();
+ friend void f1();
+ friend class Friend;
+ Foo& operator=(const Foo&);
+ ~Foo();
+ class Nested {
void f();
+ };
};
+class Friend {
+};
+
void f1();
inline void f2() {}
static const int KInt = 2;
@@ -198,25 +213,68 @@
} // namespace foo
)";
runSymbolCollector(Header, /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAreArray(
- {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
- QName("kStr"), QName("foo"), QName("foo::bar"),
- QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
- QName("foo::bar::v2"), QName("foo::baz")}));
+ EXPECT_THAT(Symbols, UnorderedElementsAreArray(
+ {QName("Foo"), QName("Foo::Foo"),
+QName("Foo::Foo"), QName("Foo::f"),
+QName("Foo::~Foo"),QName("Foo::operator="),
+QName("Foo::Nested"), QName("Foo::Nested::f"),
+QName("Friend"), QName("f1"),
+QName("f2"), QName("KInt"),
+QName("kStr"), QName("foo"),
+QName("foo::bar"), QName("foo::int32"),
+QName("foo::int32_t"), QName("foo::v1"),
+QName("foo::bar::v2"), QName("foo::baz")}));
}
TEST_F(SymbolCollectorTest, Template) {
Annotations Header(R"(
// Template is indexed, specialization and instantiation is not.
-template struct [[Tmpl]] {T x = 0;};
+template struct [[Tmpl]] {T $xdecl[[x]] = 0;};
template <> struct Tmpl {};
extern template struct Tmpl;
template struct Tmpl;
)");
runSymbolCollector(Header.code(), /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAreArray({AllOf(
- QName("Tmpl"), DeclRange(Header.range()))}));
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAreArray(
+ {AllOf(QName("Tmpl"), DeclRange(Header.range())),
+ AllOf(QName("Tmpl::x"), DeclRange(Header.range("xdecl")))}));
+}
+
+TEST_F(SymbolCollectorTest, ObjCSymbols) {
+ const std::string Header = R"(
+@interface Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName;
+@end
+
+@implementation Person
+- (void)someMethodName:(void*)name1 lastName:(void*)lName{
+ int foo;
+ ^(int param){ int bar; };
+}
+@end
+
+@interface Person (MyCategory)
+- (void)someMethodName2:(void*)name2;
+@end
+
+@implementation Person (MyCategory)
+- (void)someMethodName2:(void*)name2 {
+ int foo2;
+}
+@end
+
+@protocol MyProtocol
+- (void)someMethodName3:(void*)name3;
+@end
+ )";
+ TestFileName = "test.m";
+ runSymbolCollector(Header, /*Main=*/"", {"-fblocks"});
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ QName("Person"), QName("Person::someMethodName:lastName:"),
+ QName("MyCategory"), QName("Person::someMethodName2:"),
+ QName("MyProtocol"), QName("MyProtocol::someMethodName3:")));
}
TEST_F(SymbolCollectorTest, Locations) {
@@ -334,29 +392,30 @@
Green
};
enum class Color2 {
- Yellow // ignore
+ Yellow
};
namespace ns {
enum {
Black
};
}
)";
runSymbolCollector(Header, /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAre(QName("Red"), QName("Color"),
-QName("Green"), QName("Color2"),
-QName("ns"), QName("ns::Black")));
+ EXPECT_THAT(Symbols,
+ Uno
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added a comment. In https://reviews.llvm.org/D44954#1104497, @malaperle wrote: > >>> What scopes will non-scoped enum members have? > >> > >> Hmm. I think all of them, since you can refer them like that in code too. > >> Case #1 doesn't work but that was the case before this patch so it can > >> probably be addressed separately. I'll add some tests though! > > > > I would vote for making queries `En::A` and `A` match the enumerator, but > > **not** `::A`. The reasoning is: yes, you can reference it this way in a > > C++ file, but `workspaceSymbol` is not a real C++ resolve and I think it > > should match the outline of the code rather than the actual C++ lookup > > rules. > > E.g. I wouldn't expect it to match symbols from base classes, etc. This > > should also simplify implementation too. > > I don't have a strong opinion, so I can try this suggestion! I changed the behavior of non-scoped enums as suggested here: https://reviews.llvm.org/D47223 Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added a comment. In https://reviews.llvm.org/D44954#1104178, @ilya-biryukov wrote: > In https://reviews.llvm.org/D44954#1103664, @malaperle wrote: > > > It's probably better to consider this in a future patch. Maybe something > > like the first suggestion: vector::push_back and match both. Otherwise, I > > would think it might be a bit too verbose to have to spell out all of the > > specialization. Maybe we could allow it too. So... all of the above? :) > > > This certainly does not have to be addressed in this patch. Just wanted to > collect opinions on what the behavior we want in the long term. > My thoughts would be towards allowing only `vector::push_back` and make it > match both `push_back`s: in `vector` and `vector`. > Other cases might work too, but I wouldn't try implementing something that > matches specializations. It's just too complicated in the general case. This > will only be used in workspaceSymbol and it's fine to give a few more results > there... Sounds very reasonable. >>> What scopes will non-scoped enum members have? >> >> Hmm. I think all of them, since you can refer them like that in code too. >> Case #1 doesn't work but that was the case before this patch so it can >> probably be addressed separately. I'll add some tests though! > > I would vote for making queries `En::A` and `A` match the enumerator, but > **not** `::A`. The reasoning is: yes, you can reference it this way in a C++ > file, but `workspaceSymbol` is not a real C++ resolve and I think it should > match the outline of the code rather than the actual C++ lookup rules. > E.g. I wouldn't expect it to match symbols from base classes, etc. This > should also simplify implementation too. I don't have a strong opinion, so I can try this suggestion! Comment at: clangd/index/Index.h:160 + /// The Decl::Kind for the context of the symbol, i.e. what contains it. + Decl::Kind DeclContextKind; + /// Whether or not this is an enumerator inside a scoped enum (C++11). ioeric wrote: > malaperle wrote: > > ioeric wrote: > > > ilya-biryukov wrote: > > > > How do we use `DeclContextKind`? > > > > Why did we decide to not go with a `bool ForCompletion` instead? (I'm > > > > probably missing a conversation in the workspaceSymbol review, could > > > > you point me to those instead?) > > > > > > > > I'm asking because clang enums are very detailed and designed for use > > > > in the compiler, using them in the index seems to complicate things. > > > > It feels we don't need this level of detail in the symbols. Similar to > > > > how we don't store the whole structural type, but rely on string > > > > representation of completion label instead. > > > +1 > > > > > > `ForCompletion` sounds reasonable as the current design of index-based > > > code completion relies on assumptions about contexts. > > My thinking was that the "ForCompletion" boolean was too specific and > > tailored for one client use. I thought the Symbol information should not > > have that much hard-coded knowledge on how it would be used. It would be > > odd to have "ForWorkspaceSymbol", "ForDocumentSymbol", etc. That is why I > > was going for a slightly more detailed symbol information that opened the > > door for more arbitrary queries for symbol clients. But it complicates > > things a bit more and I'd be happy to bring back the "ForCompletion" if it > > makes more sense for now. > I think code completion, with the most complicated use of the index so far, > probably deserves a flag :P I would expect/hope other features to be less > "picky" about symbols. A high-level flag like `ForCompletion` would help keep > knowledge about filtering for code completion (e.g. enums ...) inside symbol > collector, which I think could be a win. > > FWIW, `ForCompletion` makes it sound like a symbols is collected specifically > for code completion. Maybe something like `bool SupportGlobalCompletion` > would be better? > Maybe something like bool SupportGlobalCompletion would be better? Sounds good! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
ilya-biryukov added a comment. In https://reviews.llvm.org/D44954#1103664, @malaperle wrote: > It's probably better to consider this in a future patch. Maybe something like > the first suggestion: vector::push_back and match both. Otherwise, I would > think it might be a bit too verbose to have to spell out all of the > specialization. Maybe we could allow it too. So... all of the above? :) This certainly does not have to be addressed in this patch. Just wanted to collect opinions on what the behavior we want in the long term. My thoughts would be towards allowing only `vector::push_back` and make it match both `push_back`s: in `vector` and `vector`. Other cases might work too, but I wouldn't try implementing something that matches specializations. It's just too complicated in the general case. This will only be used in workspaceSymbol and it's fine to give a few more results there... >> What scopes will non-scoped enum members have? > > Hmm. I think all of them, since you can refer them like that in code too. > Case #1 doesn't work but that was the case before this patch so it can > probably be addressed separately. I'll add some tests though! I would vote for making queries `En::A` and `A` match the enumerator, but **not** `::A`. The reasoning is: yes, you can reference it this way in a C++ file, but `workspaceSymbol` is not a real C++ resolve and I think it should match the outline of the code rather than the actual C++ lookup rules. E.g. I wouldn't expect it to match symbols from base classes, etc. This should also simplify implementation too. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
ioeric added a comment. > It's also for textDocument/documentSymbol. For this, we technically don't > need them in the static index since we could collect symbols when the > document is opened, but we also want them for workspaceSymbols so we might as > well use the same symbol collector, etc. There should be more things that > would also use symbol in main files, like Type Hierarchy. I see. Thanks! Looks like we would need to collect symbols in main file at some point. We could probably filter out main file symbols in dynamic index for code completion, but we would probably need a flag to turn off main file symbols in case this leads to a huge size increase. I would also expect different indexers (dynamic and static ) to share SymbolCollector, but it should be easily configured to behave differently via `SymbolCollector::Options`. Anyhow, let's discuss more about this when we are turning on main file symbols :) Comment at: clangd/index/Index.h:160 + /// The Decl::Kind for the context of the symbol, i.e. what contains it. + Decl::Kind DeclContextKind; + /// Whether or not this is an enumerator inside a scoped enum (C++11). malaperle wrote: > ioeric wrote: > > ilya-biryukov wrote: > > > How do we use `DeclContextKind`? > > > Why did we decide to not go with a `bool ForCompletion` instead? (I'm > > > probably missing a conversation in the workspaceSymbol review, could you > > > point me to those instead?) > > > > > > I'm asking because clang enums are very detailed and designed for use in > > > the compiler, using them in the index seems to complicate things. > > > It feels we don't need this level of detail in the symbols. Similar to > > > how we don't store the whole structural type, but rely on string > > > representation of completion label instead. > > +1 > > > > `ForCompletion` sounds reasonable as the current design of index-based code > > completion relies on assumptions about contexts. > My thinking was that the "ForCompletion" boolean was too specific and > tailored for one client use. I thought the Symbol information should not have > that much hard-coded knowledge on how it would be used. It would be odd to > have "ForWorkspaceSymbol", "ForDocumentSymbol", etc. That is why I was going > for a slightly more detailed symbol information that opened the door for more > arbitrary queries for symbol clients. But it complicates things a bit more > and I'd be happy to bring back the "ForCompletion" if it makes more sense for > now. I think code completion, with the most complicated use of the index so far, probably deserves a flag :P I would expect/hope other features to be less "picky" about symbols. A high-level flag like `ForCompletion` would help keep knowledge about filtering for code completion (e.g. enums ...) inside symbol collector, which I think could be a win. FWIW, `ForCompletion` makes it sound like a symbols is collected specifically for code completion. Maybe something like `bool SupportGlobalCompletion` would be better? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added a comment.
In https://reviews.llvm.org/D44954#1102807, @ilya-biryukov wrote:
> A few questions regarding class members. To pinpoint some interesting cases
> and agree on how we want those to behave in the long run.
>
> How do we handle template specializations? What will the qualified names of
> those instantiations be?
> I.e. how do I query for `push_back` inside a vector? Which of the following
> queries should produce a result?
>
> - `vector::push_back`. Should it match both `vector::push_back` and
> `vector::push_back` or only the first one?
> - `vector::push_back`
> - `vector::push_back`
It's probably better to consider this in a future patch. Maybe something like
the first suggestion: vector::push_back and match both. Otherwise, I would
think it might be a bit too verbose to have to spell out all of the
specialization. Maybe we could allow it too. So... all of the above? :)
> What scopes will non-scoped enum members have?
> E.g. if I have `enum En { A,B,C}`, which of the following queries will and
> won't find enumerators?
>
> - `En::A`
> - `::A`
> - `A`
Hmm. I think all of them, since you can refer them like that in code too. Case
#1 doesn't work but that was the case before this patch so it can probably be
addressed separately. I'll add some tests though!
In https://reviews.llvm.org/D44954#1103026, @ioeric wrote:
> In https://reviews.llvm.org/D44954#1101922, @malaperle wrote:
>
> > @ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make
> > sense to add a flag to disable indexing members. Could you comment on that?
> > What kind of granularity were you thinking? Would a "member" flag cover
> > both class members (member vars and functions) and enum class enumerators
> > for example? I think that would be reasonable. But I will also add symbols
> > in main files too, so another flag for that? Hmmm.
>
>
> Sam convinced me that members would still be interesting for our internal
> index service (e.g. locations of members would be useful for go-to-def). I'll
> investigate how much impact that would be by running the indexer with your
> change patched in, but I don't want to block you on that, so I'm fine with
> checking this in without any filter. We could revisit the filter design when
> needed.
>
> We actually had an option for indexing symbols in main files :) But it was
> removed as it turned out to be unused for the features clangd had at that
> point. I think it would be reasonable to add it back if we start supporting
> collecting main file symbols again. Maybe out of the scope of this patch, but
> I am interested in the use cases you have for symbols in main files, besides
> in `workspaceSymbols`?
It's also for textDocument/documentSymbol. For this, we technically don't need
them in the static index since we could collect symbols when the document is
opened, but we also want them for workspaceSymbols so we might as well use the
same symbol collector, etc. There should be more things that would also use
symbol in main files, like Type Hierarchy.
Comment at: clangd/CodeComplete.cpp:932
Req.Query = Filter->pattern();
+Req.DeclContexts = {Decl::Kind::Namespace, Decl::Kind::TranslationUnit,
+Decl::Kind::LinkageSpec, Decl::Kind::Enum};
ilya-biryukov wrote:
> malaperle wrote:
> > I want to add a comment here, but I want to make sure I understand why in
> > the first place we were not indexing symbols outside these contexts for the
> > purpose of code completion. Is it because those will be available by Sema
> > code completion anyway?
> C++ lookup rules inside classes are way more complicated than in namespaces
> and we can't possibly hope to give a decent approximation for those.
> Moreover, completion inside classes does not require any non-local
> information, so there does not seem to be a win when using the index anyway.
> So we'd rather rely on clang to do completion there, it will give way more
> useful results than any index implementation.
Makes sense. Thanks! I'll be able to document this better now with the full
picture.
Comment at: clangd/index/Index.h:160
+ /// The Decl::Kind for the context of the symbol, i.e. what contains it.
+ Decl::Kind DeclContextKind;
+ /// Whether or not this is an enumerator inside a scoped enum (C++11).
ioeric wrote:
> ilya-biryukov wrote:
> > How do we use `DeclContextKind`?
> > Why did we decide to not go with a `bool ForCompletion` instead? (I'm
> > probably missing a conversation in the workspaceSymbol review, could you
> > point me to those instead?)
> >
> > I'm asking because clang enums are very detailed and designed for use in
> > the compiler, using them in the index seems to complicate things.
> > It feels we don't need this level of detail in the symbols. Similar to how
> > we don't store the whole structural type, but rely on string representation
> > o
[PATCH] D44954: [clangd] Add "member" symbols to the index
ioeric added inline comments.
Comment at: clangd/index/SymbolCollector.cpp:113
- // We only want:
- // * symbols in namespaces or translation unit scopes (e.g. no class
- // members)
- // * enum constants in unscoped enum decl (e.g. "red" in "enum {red};")
- auto InTopLevelScope = hasDeclContext(
- anyOf(namespaceDecl(), translationUnitDecl(), linkageSpecDecl()));
- // Don't index template specializations.
+ // Don't index template specializations and expansions in main files.
auto IsSpecialization =
We should be careful when removing the `InTopLevelScope` filter. According to
clang/AST/DeclBase.h, `DeclContext` can be any of the following. For example,
indexing symbols in functions seems to be out of the scope of this patch. I
think we should keep a whitelist instead of turning them all at once. It's
unfortunately we don't have tests to catch those...
```
/// TranslationUnitDecl
/// NamespaceDecl
/// FunctionDecl
/// TagDecl
/// ObjCMethodDecl
/// ObjCContainerDecl
/// LinkageSpecDecl
/// ExportDecl
/// BlockDecl
/// OMPDeclareReductionDecl
```
For class members, I would be good to add more tests for symbol collector e.g.
more realistic class layouts with constructor, friends etc.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
ioeric added a comment. In https://reviews.llvm.org/D44954#1101922, @malaperle wrote: > @ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make > sense to add a flag to disable indexing members. Could you comment on that? > What kind of granularity were you thinking? Would a "member" flag cover both > class members (member vars and functions) and enum class enumerators for > example? I think that would be reasonable. But I will also add symbols in > main files too, so another flag for that? Hmmm. Sam convinced me that members would still be interesting for our internal index service (e.g. locations of members would be useful for go-to-def). I'll investigate how much impact that would be by running the indexer with your change patched in, but I don't want to block you on that, so I'm fine with checking this in without any filter. We could revisit the filter design when needed. We actually had an option for indexing symbols in main files :) But it was removed as it turned out to be unused for the features clangd had at that point. I think it would be reasonable to add it back if we start supporting collecting main file symbols again. Maybe out of the scope of this patch, but I am interested in the use cases you have for symbols in main files, besides in `workspaceSymbols`? Comment at: clangd/index/Index.h:160 + /// The Decl::Kind for the context of the symbol, i.e. what contains it. + Decl::Kind DeclContextKind; + /// Whether or not this is an enumerator inside a scoped enum (C++11). ilya-biryukov wrote: > How do we use `DeclContextKind`? > Why did we decide to not go with a `bool ForCompletion` instead? (I'm > probably missing a conversation in the workspaceSymbol review, could you > point me to those instead?) > > I'm asking because clang enums are very detailed and designed for use in the > compiler, using them in the index seems to complicate things. > It feels we don't need this level of detail in the symbols. Similar to how we > don't store the whole structural type, but rely on string representation of > completion label instead. +1 `ForCompletion` sounds reasonable as the current design of index-based code completion relies on assumptions about contexts. Comment at: clangd/index/Index.h:162 + /// Whether or not this is an enumerator inside a scoped enum (C++11). + bool InScopedEnum = false; /// A brief description of the symbol that can be displayed in the completion In case we do need these fields, I think they should go into `index::SymbolInfo` (i.e. `SymInfo` above)? As Ilya said, `Symbol` might be the wrong level to put these. And we would want them in index-while-build anyway. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
ilya-biryukov added a comment.
A few questions regarding class members. To pinpoint some interesting cases and
agree on how we want those to behave in the long run.
How do we handle template specializations? What will the qualified names of
those instantiations be?
I.e. how do I query for `push_back` inside a vector? Which of the following
queries should produce a result?
- `vector::push_back`. Should it match both `vector::push_back` and
`vector::push_back` or only the first one?
- `vector::push_back`
- `vector::push_back`
What scopes will non-scoped enum members have?
E.g. if I have `enum En { A,B,C}`, which of the following queries will and
won't find enumerators?
- `En::A`
- `::A`
- `A`
Comment at: clangd/CodeComplete.cpp:932
Req.Query = Filter->pattern();
+Req.DeclContexts = {Decl::Kind::Namespace, Decl::Kind::TranslationUnit,
+Decl::Kind::LinkageSpec, Decl::Kind::Enum};
malaperle wrote:
> I want to add a comment here, but I want to make sure I understand why in the
> first place we were not indexing symbols outside these contexts for the
> purpose of code completion. Is it because those will be available by Sema
> code completion anyway?
C++ lookup rules inside classes are way more complicated than in namespaces and
we can't possibly hope to give a decent approximation for those.
Moreover, completion inside classes does not require any non-local information,
so there does not seem to be a win when using the index anyway.
So we'd rather rely on clang to do completion there, it will give way more
useful results than any index implementation.
Comment at: clangd/index/Index.h:160
+ /// The Decl::Kind for the context of the symbol, i.e. what contains it.
+ Decl::Kind DeclContextKind;
+ /// Whether or not this is an enumerator inside a scoped enum (C++11).
How do we use `DeclContextKind`?
Why did we decide to not go with a `bool ForCompletion` instead? (I'm probably
missing a conversation in the workspaceSymbol review, could you point me to
those instead?)
I'm asking because clang enums are very detailed and designed for use in the
compiler, using them in the index seems to complicate things.
It feels we don't need this level of detail in the symbols. Similar to how we
don't store the whole structural type, but rely on string representation of
completion label instead.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added a comment. @ioeric You mentioned in https://reviews.llvm.org/D46751 that it would make sense to add a flag to disable indexing members. Could you comment on that? What kind of granularity were you thinking? Would a "member" flag cover both class members (member vars and functions) and enum class enumerators for example? I think that would be reasonable. But I will also add symbols in main files too, so another flag for that? Hmmm. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44954 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44954: [clangd] Add "member" symbols to the index
malaperle added inline comments.
Comment at: clangd/CodeComplete.cpp:932
Req.Query = Filter->pattern();
+Req.DeclContexts = {Decl::Kind::Namespace, Decl::Kind::TranslationUnit,
+Decl::Kind::LinkageSpec, Decl::Kind::Enum};
I want to add a comment here, but I want to make sure I understand why in the
first place we were not indexing symbols outside these contexts for the purpose
of code completion. Is it because those will be available by Sema code
completion anyway?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44954
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
