[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added a comment.

Build failures seem unrelated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG535f34dd80c2: [index][clangd] Consider labels when indexing 
function bodies (authored by ckandeler).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

Files:
  clang-tools-extra/clangd/unittests/XRefsTests.cpp
  clang/lib/Index/IndexBody.cpp
  clang/unittests/Index/IndexTests.cpp


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  Contains(AllOf(QName("theLabel"), WrittenAt(Position(3, 16)),
+ DeclAt(Position(4, 11);
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,17 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC);
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -125,6 +125,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  Contains(AllOf(QName("theLabel"), WrittenAt(Position(3, 16)),
+ DeclAt(Position(4, 11);
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,17 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC);
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -125,6 +125,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+   

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Nathan Ridge via Phabricator via cfe-commits]

nridge accepted this revision.
nridge added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler updated this revision to Diff 545551.
ckandeler added a comment.

Addressed another review comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

Files:
  clang-tools-extra/clangd/unittests/XRefsTests.cpp
  clang/lib/Index/IndexBody.cpp
  clang/unittests/Index/IndexTests.cpp


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  Contains(AllOf(QName("theLabel"), WrittenAt(Position(3, 16)),
+ DeclAt(Position(4, 11);
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,17 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC);
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -125,6 +125,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  Contains(AllOf(QName("theLabel"), WrittenAt(Position(3, 16)),
+ DeclAt(Position(4, 11);
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,17 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC);
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -125,6 +125,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )c

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D150124#4542032 , @kadircet wrote:

> btw, thanks a lot Nathan for taking good care of clangd, I don't think I say 
> that enough

I'm happy to help!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang/lib/Index/IndexBody.cpp:150
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }

ckandeler wrote:
> kadircet wrote:
> > nridge wrote:
> > > `NameReference` was introduced in 
> > > https://github.com/llvm/llvm-project/commit/e7eb27a9a0edd859de49bcc9af7ca27dbb435886
> > >  to handle the somewhat unique situation with constructors and 
> > > destructors where the constructor/destructor references the class by name 
> > > but semantically denotes a separate entity.
> > > 
> > > Why is that applicable here?
> > > 
> > > Note that `handleReference()` will automatically add 
> > > `SymbolRole::Reference` 
> > > [here](https://searchfox.org/llvm/rev/cea72fe34194d58ac1ba9485ee9c9a63cf98a4e6/clang/lib/Index/IndexingContext.cpp#404).
> > +1 we shouldn't be setting namereference here.
> So set nothing at all here?
Yes, I think we should pass a default constructed `SymbolRoleSet()` here, and 
`handleReference()` will add `SymbolRole::Reference` into it automatically.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added inline comments.



Comment at: clang/lib/Index/IndexBody.cpp:150
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }

kadircet wrote:
> nridge wrote:
> > `NameReference` was introduced in 
> > https://github.com/llvm/llvm-project/commit/e7eb27a9a0edd859de49bcc9af7ca27dbb435886
> >  to handle the somewhat unique situation with constructors and destructors 
> > where the constructor/destructor references the class by name but 
> > semantically denotes a separate entity.
> > 
> > Why is that applicable here?
> > 
> > Note that `handleReference()` will automatically add 
> > `SymbolRole::Reference` 
> > [here](https://searchfox.org/llvm/rev/cea72fe34194d58ac1ba9485ee9c9a63cf98a4e6/clang/lib/Index/IndexingContext.cpp#404).
> +1 we shouldn't be setting namereference here.
So set nothing at all here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

sorry for missing this review. Nathan's explanation around `targetDecl` vs 
`findRefs` is absolutely right (btw, thanks a lot Nathan for taking good care 
of clangd, I don't think I say that enough). hopefully one day we can switch 
clangd's usage of libindex to our internal find target/explicitrefs 
implementation, but until then we actually need to address such things in both 
places to make sure functionality is actually consistent.




Comment at: clang/lib/Index/IndexBody.cpp:150
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }

nridge wrote:
> `NameReference` was introduced in 
> https://github.com/llvm/llvm-project/commit/e7eb27a9a0edd859de49bcc9af7ca27dbb435886
>  to handle the somewhat unique situation with constructors and destructors 
> where the constructor/destructor references the class by name but 
> semantically denotes a separate entity.
> 
> Why is that applicable here?
> 
> Note that `handleReference()` will automatically add `SymbolRole::Reference` 
> [here](https://searchfox.org/llvm/rev/cea72fe34194d58ac1ba9485ee9c9a63cf98a4e6/clang/lib/Index/IndexingContext.cpp#404).
+1 we shouldn't be setting namereference here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler updated this revision to Diff 545054.
ckandeler added a comment.

Addressed review comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

Files:
  clang-tools-extra/clangd/unittests/XRefsTests.cpp
  clang/lib/Index/IndexBody.cpp
  clang/unittests/Index/IndexTests.cpp


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  Contains(AllOf(QName("theLabel"), WrittenAt(Position(3, 16)),
+ DeclAt(Position(4, 11);
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,18 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -125,6 +125,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+  Contains(AllOf(QName("theLabel"), WrittenAt(Position(3, 16)),
+ DeclAt(Position(4, 11);
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,18 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -125,6 +125,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+   

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D150124#4329163 , @ilya-biryukov 
wrote:

> In `clangd` we also have `findExplicitReferences` and `targetDecl` functions 
> that seem to handle the `GoToStmt`.
> In fact, I believe they are used in `findDocumentHighlights`, so I'm not sure 
> why your test did not work before this patch.
> findDocumentHighlights

I can explain this part: `findDocumentHighlights` uses `targetDecl` to 
associate the input cursor position with a symbol, but then it uses `findRefs` 
(which uses `libIndex`) to locate //other// usages of the symbol in the file.




Comment at: clang/lib/Index/IndexBody.cpp:150
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }

`NameReference` was introduced in 
https://github.com/llvm/llvm-project/commit/e7eb27a9a0edd859de49bcc9af7ca27dbb435886
 to handle the somewhat unique situation with constructors and destructors 
where the constructor/destructor references the class by name but semantically 
denotes a separate entity.

Why is that applicable here?

Note that `handleReference()` will automatically add `SymbolRole::Reference` 
[here](https://searchfox.org/llvm/rev/cea72fe34194d58ac1ba9485ee9c9a63cf98a4e6/clang/lib/Index/IndexingContext.cpp#404).



Comment at: clang/unittests/Index/IndexTests.cpp:233
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, AllOf(Contains(AllOf(QName("theLabel"),
+   WrittenAt(Position(3, 16)),

I think the outer `AllOf()` here is a no-op, since it has only one argument. 
(`AllOf(x, y, z)` means "each of the matchers `x`, `y`, and `z` match").


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added a comment.

ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler updated this revision to Diff 521577.
ckandeler marked 2 inline comments as done.
ckandeler added a comment.

Addressed review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

Files:
  clang-tools-extra/clangd/unittests/XRefsTests.cpp
  clang/lib/Index/IndexBody.cpp
  clang/unittests/Index/IndexTests.cpp


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, AllOf(Contains(AllOf(QName("theLabel"),
+   WrittenAt(Position(3, 16)),
+   DeclAt(Position(4, 11));
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,18 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -127,6 +127,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);


Index: clang/unittests/Index/IndexTests.cpp
===
--- clang/unittests/Index/IndexTests.cpp
+++ clang/unittests/Index/IndexTests.cpp
@@ -218,6 +218,28 @@
   EXPECT_THAT(Index->Symbols, Not(Contains(QName("bar";
 }
 
+TEST(IndexTest, IndexLabels) {
+  std::string Code = R"cpp(
+int main() {
+  goto theLabel;
+  theLabel:
+return 1;
+}
+  )cpp";
+  auto Index = std::make_shared();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, AllOf(Contains(AllOf(QName("theLabel"),
+   WrittenAt(Position(3, 16)),
+   DeclAt(Position(4, 11));
+
+  Opts.IndexFunctionLocals = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(std::make_unique(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Not(Contains(QName("theLabel";
+}
+
 TEST(IndexTest, IndexExplicitTemplateInstantiation) {
   std::string Code = R"cpp(
 template 
Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,18 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+ParentDC,
+unsigned(SymbolRole::NameReference));
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols())
+  return IndexCtx.handleDecl(S->getDecl());
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler added a comment.

In D150124#4329163 , @ilya-biryukov 
wrote:

> In `clangd` we also have `findExplicitReferences` and `targetDecl` functions 
> that seem to handle the `GoToStmt`.

According to the commit message of the patch that added this, it was for "go to 
definition" functionality (which did indeed work already).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a reviewer: kadircet.
ilya-biryukov added a comment.

In `clangd` we also have `findExplicitReferences` and `targetDecl` functions 
that seem to handle the `GoToStmt`.
In fact, I believe they are used in `findDocumentHighlights`, so I'm not sure 
why your test did not work before this patch.

I have also not looked into `libIndex` for quite a long time, so looping in 
@kadircet for a second pair of eyes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks! This looks like a useful addition.

Could you add a test into `libIndex` for this too? Just for the sake of having 
better coverage, some folks probably don't run `clang-tools-extra` tests.
Also, could we add a test that a label does not get indexed if local function 
indexing is off?




Comment at: clang/lib/Index/IndexBody.cpp:148
+  bool VisitGotoStmt(GotoStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+  return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,

`IndexingContext::handleReference` already has this check, we should call 
`handleReference` unconditionally.





Comment at: clang/lib/Index/IndexBody.cpp:158
+if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+  return IndexCtx.handleReference(S->getDecl(), S->getIdentLoc(), Parent,
+  ParentDC, {});

We should call `handleDecl`, indexing output can differentiate between a 
declarations and a references.
`LabelStmt` is exactly the node that declares a `LabelDecl`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150124/new/

https://reviews.llvm.org/D150124

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

[Christian Kandeler via Phabricator via cfe-commits]

ckandeler created this revision.
ckandeler added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
ckandeler requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added projects: clang, clang-tools-extra.

It's valuable to have document highlights for labels and be able to find
references to them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150124

Files:
  clang-tools-extra/clangd/unittests/XRefsTests.cpp
  clang/lib/Index/IndexBody.cpp


Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,23 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+  return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+  ParentDC,
+  unsigned(SymbolRole::NameReference));
+}
+return true;
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+  return IndexCtx.handleReference(S->getDecl(), S->getIdentLoc(), Parent,
+  ParentDC, {});
+}
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -127,6 +127,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);


Index: clang/lib/Index/IndexBody.cpp
===
--- clang/lib/Index/IndexBody.cpp
+++ clang/lib/Index/IndexBody.cpp
@@ -144,6 +144,23 @@
 Parent, ParentDC, Roles, Relations, E);
   }
 
+  bool VisitGotoStmt(GotoStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+  return IndexCtx.handleReference(S->getLabel(), S->getLabelLoc(), Parent,
+  ParentDC,
+  unsigned(SymbolRole::NameReference));
+}
+return true;
+  }
+
+  bool VisitLabelStmt(LabelStmt *S) {
+if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
+  return IndexCtx.handleReference(S->getDecl(), S->getIdentLoc(), Parent,
+  ParentDC, {});
+}
+return true;
+  }
+
   bool VisitMemberExpr(MemberExpr *E) {
 SourceLocation Loc = E->getMemberLoc();
 if (Loc.isInvalid())
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -127,6 +127,13 @@
   [Foo [[x]]:2 [[^y]]:4];
 }
   )cpp",
+  R"cpp( // Label
+int main() {
+  goto [[^theLabel]];
+  [[theLabel]]:
+return 1;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits