[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE349033: [clangd] Refine the way of checking a declaration
is referenced by the written… (authored by hokein, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D55191?vs=177190=178038#toc
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55191/new/
https://reviews.llvm.org/D55191
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -139,21 +139,19 @@
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
if (Loc == SearchedLocation) {
- // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
- auto hasImplicitExpr = [](const Expr *E) {
-if (!E || E->child_begin() == E->child_end())
+ auto isImplicitExpr = [](const Expr *E) {
+if (!E)
return false;
-// Use the first child is good enough for most cases -- normally the
-// expression returned by handleDeclOccurence contains exactly one
-// child expression.
-const auto *FirstChild = *E->child_begin();
-return isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild);
+// We assume that a constructor expression is implict (was inserted by
+// clang) if it has an invalid paren/brace location, since such
+// experssion is impossible to write down.
+if (const auto *CtorExpr = dyn_cast(E))
+ return CtorExpr->getNumArgs() > 0 &&
+ CtorExpr->getParenOrBraceRange().isInvalid();
+return isa(E);
};
- bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE);
+ bool IsExplicit = !isImplicitExpr(ASTNode.OrigE);
// Find and add definition declarations (for GoToDefinition).
// We don't use parameter `D`, as Parameter `D` is the canonical
// declaration, which is the first declaration of a redeclarable
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1225,6 +1225,53 @@
}
}
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
+ R"cpp(
+ struct Foo { Foo* [self]() const; };
+ void f() {
+if (Foo* T = foo.[^self]()) {} // Foo member call expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo { Foo(int); };
+ Foo f() {
+int [b];
+return [^b]; // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo {};
+ void g(Foo);
+ Foo [f]();
+ void call() {
+g([^f]()); // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ void [foo](int);
+ void [foo](double);
+
+ namespace ns {
+ using ::[fo^o];
+ }
+ )cpp",
+ };
+ for (const char *Test : Tests) {
+Annotations T(Test);
+auto AST = TestTU::withCode(T.code()).build();
+std::vector> ExpectedLocations;
+for (const auto : T.ranges())
+ ExpectedLocations.push_back(RangeIs(R));
+EXPECT_THAT(findReferences(AST, T.point()),
+ElementsAreArray(ExpectedLocations))
+<< Test;
+ }
+}
+
TEST(FindReferences, NeedsIndex) {
const char *Header = "int foo();";
Annotations Main("int main() { [[f^oo]](); }");
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -139,21 +139,19 @@
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
if (Loc == SearchedLocation) {
- // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
- auto hasImplicitExpr = [](const Expr *E) {
-if (!E || E->child_begin() == E->child_end())
+ auto isImplicitExpr = [](const Expr *E) {
+if (!E)
return false;
-// Use the first child is good enough for most cases -- normally the
-// expression returned by handleDeclOccurence contains exactly one
-// child expression.
-const auto *FirstChild = *E->child_begin();
-return isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild);
+// We assume that a constructor expression is implict (was inserted by
+// clang) if it has an invalid paren/brace location, since such
+// experssion is impossible to write down.
+if (const auto *CtorExpr = dyn_cast(E))
+ return CtorExpr->getNumArgs() > 0 &&
+ CtorExpr->getParenOrBraceRange().isInvalid();
+return isa(E);
};
- bool IsExplicit =
[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. Many thanks for the fix, really important to get those cases right.
Comment at: unittests/clangd/XRefsTests.cpp:1228
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
hokein wrote:
> ilya-biryukov wrote:
> > I'm missing what does this test actually tests.
> > The absence of implicit references (I guess constructor expressions)?
> This test tests the cases where symbol is being marked implicit incorrectly,
> which will result in no result in xref.
Thanks for clarification! The tested cases were so simple that I though they
worked before.
(Which shows this change is very important, IMHO)
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55191/new/
https://reviews.llvm.org/D55191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.
hokein added a comment.
> What are the cases we're trying to filter out? Only implicit constructor or
> anything else?
I think implicit constructor is the only case we care about. We also looked
through the caller implementation of `handleDeclOccurence`, it is safe to
assume it.
Comment at: unittests/clangd/XRefsTests.cpp:1228
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
ilya-biryukov wrote:
> I'm missing what does this test actually tests.
> The absence of implicit references (I guess constructor expressions)?
This test tests the cases where symbol is being marked implicit incorrectly,
which will result in no result in xref.
Comment at: unittests/clangd/XRefsTests.cpp:1259
+ namespace ns {
+ using [fo^o];
+ }
ilya-biryukov wrote:
> Shouldn't the `usings` always be qualified? Isn't this a compiler error?
Yes, this is a compiler error, but clang's error recovery can handle it. Fixed.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55191/new/
https://reviews.llvm.org/D55191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.
hokein updated this revision to Diff 177190.
hokein marked 4 inline comments as done.
hokein added a comment.
Update based on our offline discussion.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55191/new/
https://reviews.llvm.org/D55191
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1225,6 +1225,53 @@
}
}
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
+ R"cpp(
+ struct Foo { Foo* [self]() const; };
+ void f() {
+if (Foo* T = foo.[^self]()) {} // Foo member call expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo { Foo(int); };
+ Foo f() {
+int [b];
+return [^b]; // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo {};
+ void g(Foo);
+ Foo [f]();
+ void call() {
+g([^f]()); // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ void [foo](int);
+ void [foo](double);
+
+ namespace ns {
+ using ::[fo^o];
+ }
+ )cpp",
+ };
+ for (const char *Test : Tests) {
+Annotations T(Test);
+auto AST = TestTU::withCode(T.code()).build();
+std::vector> ExpectedLocations;
+for (const auto : T.ranges())
+ ExpectedLocations.push_back(RangeIs(R));
+EXPECT_THAT(findReferences(AST, T.point()),
+ElementsAreArray(ExpectedLocations))
+<< Test;
+ }
+}
+
TEST(FindReferences, NeedsIndex) {
const char *Header = "int foo();";
Annotations Main("int main() { [[f^oo]](); }");
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -139,21 +139,19 @@
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
if (Loc == SearchedLocation) {
- // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
- auto hasImplicitExpr = [](const Expr *E) {
-if (!E || E->child_begin() == E->child_end())
+ auto isImplicitExpr = [](const Expr *E) {
+if (!E)
return false;
-// Use the first child is good enough for most cases -- normally the
-// expression returned by handleDeclOccurence contains exactly one
-// child expression.
-const auto *FirstChild = *E->child_begin();
-return isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild);
+// We assume that a constructor expression is implict (was inserted by
+// clang) if it has an invalid paren/brace location, since such
+// experssion is impossible to write down.
+if (const auto *CtorExpr = dyn_cast(E))
+ return CtorExpr->getNumArgs() > 0 &&
+ CtorExpr->getParenOrBraceRange().isInvalid();
+return isa(E);
};
- bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE);
+ bool IsExplicit = !isImplicitExpr(ASTNode.OrigE);
// Find and add definition declarations (for GoToDefinition).
// We don't use parameter `D`, as Parameter `D` is the canonical
// declaration, which is the first declaration of a redeclarable
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1225,6 +1225,53 @@
}
}
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
+ R"cpp(
+ struct Foo { Foo* [self]() const; };
+ void f() {
+if (Foo* T = foo.[^self]()) {} // Foo member call expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo { Foo(int); };
+ Foo f() {
+int [b];
+return [^b]; // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo {};
+ void g(Foo);
+ Foo [f]();
+ void call() {
+g([^f]()); // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ void [foo](int);
+ void [foo](double);
+
+ namespace ns {
+ using ::[fo^o];
+ }
+ )cpp",
+ };
+ for (const char *Test : Tests) {
+Annotations T(Test);
+auto AST = TestTU::withCode(T.code()).build();
+std::vector> ExpectedLocations;
+for (const auto : T.ranges())
+ ExpectedLocations.push_back(RangeIs(R));
+EXPECT_THAT(findReferences(AST, T.point()),
+ElementsAreArray(ExpectedLocations))
+<< Test;
+ }
+}
+
TEST(FindReferences, NeedsIndex) {
const char *Header = "int foo();";
Annotations Main("int main() { [[f^oo]](); }");
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -139,21
[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.
ilya-biryukov added a comment.
The **idea** of using the AST-based approach here was really nice, it was less
expensive and seemed to clearly look at the semantic.
I wonder if there's a way to keep it on the AST level, without looking at the
source locations.
What are the cases we're trying to filter out? Only implicit constructor or
anything else?
Comment at: unittests/clangd/XRefsTests.cpp:1228
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
I'm missing what does this test actually tests.
The absence of implicit references (I guess constructor expressions)?
Comment at: unittests/clangd/XRefsTests.cpp:1259
+ namespace ns {
+ using [fo^o];
+ }
Shouldn't the `usings` always be qualified? Isn't this a compiler error?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55191/new/
https://reviews.llvm.org/D55191
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D55191: [clangd] Refine the way of checking a declaration is referenced by the written code.
hokein created this revision.
hokein added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric.
The previous solution (checking the AST) is not a reliable way to
determine whether a declaration is explicitly referenced by the source
code, we are still missing a few cases.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D55191
Files:
clangd/XRefs.cpp
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -1225,6 +1225,53 @@
}
}
+TEST(FindReferences, ExplicitSymbols) {
+ const char *Tests[] = {
+ R"cpp(
+ struct Foo { Foo* [self]() const; };
+ void f() {
+if (Foo* T = foo.[^self]()) {} // Foo member call expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo { Foo(int); };
+ Foo f() {
+int [b];
+return [^b]; // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ struct Foo {};
+ void g(Foo);
+ Foo [f]();
+ void call() {
+g([^f]()); // Foo constructor expr.
+ }
+ )cpp",
+
+ R"cpp(
+ void [foo](int);
+ void [foo](double);
+
+ namespace ns {
+ using [fo^o];
+ }
+ )cpp",
+ };
+ for (const char *Test : Tests) {
+Annotations T(Test);
+auto AST = TestTU::withCode(T.code()).build();
+std::vector> ExpectedLocations;
+for (const auto : T.ranges())
+ ExpectedLocations.push_back(RangeIs(R));
+EXPECT_THAT(findReferences(AST, T.point()),
+ElementsAreArray(ExpectedLocations))
+<< Test;
+ }
+}
+
TEST(FindReferences, NeedsIndex) {
const char *Header = "int foo();";
Annotations Main("int main() { [[f^oo]](); }");
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -139,30 +139,30 @@
SourceLocation Loc,
index::IndexDataConsumer::ASTNodeInfo ASTNode) override {
if (Loc == SearchedLocation) {
- // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr).
- auto hasImplicitExpr = [](const Expr *E) {
-if (!E || E->child_begin() == E->child_end())
+ // Check whether the give ND is explicitly referenced by the written
+ // identifier at Loc in the source code.
+ auto IsWrittenInCode = [this](const NamedDecl *ND, SourceLocation Loc) {
+if (!ND)
return false;
-// Use the first child is good enough for most cases -- normally the
-// expression returned by handleDeclOccurence contains exactly one
-// child expression.
-const auto *FirstChild = *E->child_begin();
-return isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild) ||
- isa(FirstChild);
+auto = AST.getSourceManager();
+auto FileIDAndOffset = SM.getDecomposedLoc(Loc);
+auto Code = SM.getBufferData(FileIDAndOffset.first);
+auto IdentifierLength =
+clang::Lexer::MeasureTokenLength(Loc, SM, AST.getLangOpts());
+StringRef WrittenName =
+Code.substr(FileIDAndOffset.second, IdentifierLength);
+return WrittenName == ND->getDeclName().getAsString();
};
-
- bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE);
+ bool WrittenInCode = IsWrittenInCode(dyn_cast(D), Loc);
// Find and add definition declarations (for GoToDefinition).
// We don't use parameter `D`, as Parameter `D` is the canonical
// declaration, which is the first declaration of a redeclarable
// declaration, and it could be a forward declaration.
if (const auto *Def = getDefinition(D)) {
-Decls[Def] |= IsExplicit;
+Decls[Def] |= WrittenInCode;
} else {
// Couldn't find a definition, fall back to use `D`.
-Decls[D] |= IsExplicit;
+Decls[D] |= WrittenInCode;
}
}
return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
