[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371976: Implement semantic selections. (authored by 
usaxena95, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67358?vs=220303=220307#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67358

Files:
  clang-tools-extra/trunk/clangd/CMakeLists.txt
  clang-tools-extra/trunk/clangd/SemanticSelection.cpp
  clang-tools-extra/trunk/clangd/SemanticSelection.h
  clang-tools-extra/trunk/clangd/unittests/CMakeLists.txt
  clang-tools-extra/trunk/clangd/unittests/SemanticSelectionTests.cpp

Index: clang-tools-extra/trunk/clangd/CMakeLists.txt
===
--- clang-tools-extra/trunk/clangd/CMakeLists.txt
+++ clang-tools-extra/trunk/clangd/CMakeLists.txt
@@ -66,6 +66,7 @@
   RIFF.cpp
   Selection.cpp
   SemanticHighlighting.cpp
+  SemanticSelection.cpp
   SourceCode.cpp
   QueryDriverDatabase.cpp
   Threading.cpp
Index: clang-tools-extra/trunk/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/trunk/clangd/SemanticSelection.cpp
+++ clang-tools-extra/trunk/clangd/SemanticSelection.cpp
@@ -0,0 +1,64 @@
+//===--- SemanticSelection.cpp ---*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "SemanticSelection.h"
+#include "ParsedAST.h"
+#include "Protocol.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+// Adds Range \p R to the Result if it is distinct from the last added Range.
+// Assumes that only consecutive ranges can coincide.
+void addIfDistinct(const Range , std::vector ) {
+  if (Result.empty() || Result.back() != R) {
+Result.push_back(R);
+  }
+}
+} // namespace
+
+llvm::Expected> getSemanticRanges(ParsedAST ,
+ Position Pos) {
+  std::vector Result;
+  const auto  = AST.getSourceManager();
+  const auto  = AST.getASTContext().getLangOpts();
+
+  auto FID = SM.getMainFileID();
+  auto Offset = positionToOffset(SM.getBufferData(FID), Pos);
+  if (!Offset) {
+return Offset.takeError();
+  }
+
+  // Get node under the cursor.
+  SelectionTree ST(AST.getASTContext(), AST.getTokens(), *Offset);
+  for (const auto *Node = ST.commonAncestor(); Node != nullptr;
+   Node = Node->Parent) {
+if (const Decl *D = Node->ASTNode.get()) {
+  if (llvm::isa(D)) {
+break;
+  }
+}
+
+auto SR = toHalfOpenFileRange(SM, LangOpts, Node->ASTNode.getSourceRange());
+if (!SR.hasValue() || SM.getFileID(SR->getBegin()) != SM.getMainFileID()) {
+  continue;
+}
+Range R;
+R.start = sourceLocToPosition(SM, SR->getBegin());
+R.end = sourceLocToPosition(SM, SR->getEnd());
+addIfDistinct(R, Result);
+  }
+  return Result;
+}
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/trunk/clangd/SemanticSelection.h
===
--- clang-tools-extra/trunk/clangd/SemanticSelection.h
+++ clang-tools-extra/trunk/clangd/SemanticSelection.h
@@ -0,0 +1,32 @@
+//===--- SemanticSelection.h -*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Features for giving interesting semantic ranges around the cursor.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICSELECTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICSELECTION_H
+#include "ParsedAST.h"
+#include "Protocol.h"
+#include "llvm/Support/Error.h"
+#include 
+namespace clang {
+namespace clangd {
+
+/// Returns the list of all interesting ranges around the Position \p Pos.
+/// The interesting ranges corresponds to the AST nodes in the SelectionTree
+/// containing \p Pos.
+/// Any range in the result strictly contains all the previous ranges in the
+/// result. front() is the inner most range. back() is the outermost range.
+llvm::Expected> getSemanticRanges(ParsedAST ,
+ Position Pos);
+} // namespace clangd
+} // namespace clang
+
+#endif // 

[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 220303.
usaxena95 marked 3 inline comments as done.
usaxena95 added a comment.

Resolved comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/SemanticSelection.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -0,0 +1,143 @@
+//===-- SemanticSelectionTests.cpp  *- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "Matchers.h"
+#include "Protocol.h"
+#include "SemanticSelection.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAreArray;
+
+TEST(SemanticSelection, All) {
+  const char *Tests[] = {
+  R"cpp( // Single statement in a function body.
+[[void func() [[{
+  int v = [[1^00;]]
+}
+  )cpp",
+  R"cpp( // Expression
+[[void func() [[{
+  int a = 1;
+  // int v = (10 + 2) * (a + a);
+  int v = (10^]] + 2]])]] * (a + a);]]
+}
+  )cpp",
+  R"cpp( // Function call.
+int add(int x, int y) { return x + y; }
+[[void callee() [[{
+  // int res = add(11, 22);
+  int res = [[add([[1^1]], 22);]]
+}
+  )cpp",
+  R"cpp( // Tricky macros.
+#define MUL ) * (
+[[void func() [[{
+  // int var = (4 + 15 MUL 6 + 10);
+  int var = ([[4 + [[1^5 MUL]] 6 + 10);]]
+}
+   )cpp",
+  R"cpp( // Cursor inside a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HASH(2^3]] + 34]]);]]
+}
+   )cpp",
+  R"cpp( // Cursor on a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HA^SH(23);]]
+}
+   )cpp",
+  R"cpp( // Multiple declaration.
+[[void func() [[{
+  int var1, var^2]], var3;]]
+}
+   )cpp",
+  R"cpp( // Before comment.
+[[void func() [[{
+  int var1 = 1;
+  int var2 = var1]]^ /*some comment*/ + 41;]]
+}
+   )cpp",
+  // Empty file.
+  "^",
+  // FIXME: We should get the whole DeclStmt as a range.
+  R"cpp( // Single statement in TU.
+[[int v = [[1^00;
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.
+void func() {
+  int v = 100 + 100^;
+}
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor in between spaces.
+void func() {
+  int v = 100 + ^  100;
+}
+  )cpp",
+  // Structs.
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = AAA::BBB::c^cc]]();]]
+}
+  )cpp",
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = [[AA^A]]::]]BBB::]]ccc]]();]]
+}
+  )cpp",
+  R"cpp( // Inside struct.
+struct A { static int a(); };
+[[struct B { 
+  [[static int b() [[{
+[[return 1^1]] + 2;
+  }
+}]];
+  )cpp",
+  // Namespaces.
+  R"cpp( 
+[[namespace nsa { 
+  [[namespace nsb { 
+static int ccc();
+[[void func() [[{
+  // int x = nsa::nsb::ccc();
+  int x = nsa::nsb::cc^c]]();]]
+}
+  }]]
+}]]
+  )cpp",
+
+  };
+
+  for (const char *Test : Tests) {
+auto T = Annotations(Test);
+auto AST = TestTU::withCode(T.code()).build();
+EXPECT_THAT(llvm::cantFail(getSemanticRanges(AST, T.point())),
+ElementsAreArray(T.ranges()))
+<< Test;
+  }
+}
+} // namespace
+} // namespace clangd
+} // namespace 

[PATCH] D67358: [clangd] Implement semantic selections.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D67358#1670810 , @nridge wrote:

> Which LSP feature is this related to?


selectionRange:

- 
https://github.com/microsoft/vscode-languageserver-node/blob/master/protocol/src/protocol.selectionRange.ts
- https://github.com/microsoft/language-server-protocol/issues/613

It hasn't been published/released yet, but AIUI it will be soon. (Thus no 
mention of it being an extension everywhere).
I think @usaxena95 is working on a followup to add the LSP bindings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358



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

[PATCH] D67358: [clangd] Implement semantic selections.

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Which LSP feature is this related to?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358



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

[PATCH] D67358: [clangd] Implement semantic selections.

[Sam McCall via Phabricator via cfe-commits]

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

Nice!

Please make sure you run clang-format (I think it'll add missing newlines at 
EOF)




Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:22
+// Assumes that only consecutive ranges can coincide.
+void addIfDistinct(const Range , std::vector *Result) {
+  if (Result->empty() || Result->back() != R) {

nit: it's idiomatic in LLVM to pass mutable references rather than pointers 
where possible



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52
+auto SR = toHalfOpenFileRange(SM, LangOpts, 
Node->ASTNode.getSourceRange());
+if (!SR.hasValue()) {
+  continue;

`|| SM.getFileID(SR->getBegin()) != SM.getMainFileID()`

(begin and end are guaranteed to be the same file, but it may not be the main 
file)



Comment at: clang-tools-extra/clangd/SemanticSelection.h:22
+
+// Returns the list of all interesting ranges around the Position \p Pos.
+// The interesting ranges corresponds to the AST nodes in the SelectionTree

nit: if you intend to use doxygen comments (with `\p` syntax) you probably want 
`///` so doxygen will render them


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358



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

[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 220075.
usaxena95 added a comment.

Remove extraneous namespace comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/SemanticSelection.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -0,0 +1,143 @@
+//===-- SemanticSelectionTests.cpp  *- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "Matchers.h"
+#include "Protocol.h"
+#include "SemanticSelection.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAreArray;
+
+TEST(SemanticSelection, All) {
+  const char *Tests[] = {
+  R"cpp( // Single statement in a function body.
+[[void func() [[{
+  int v = [[1^00;]]
+}
+  )cpp",
+  R"cpp( // Expression
+[[void func() [[{
+  int a = 1;
+  // int v = (10 + 2) * (a + a);
+  int v = (10^]] + 2]])]] * (a + a);]]
+}
+  )cpp",
+  R"cpp( // Function call.
+int add(int x, int y) { return x + y; }
+[[void callee() [[{
+  // int res = add(11, 22);
+  int res = [[add([[1^1]], 22);]]
+}
+  )cpp",
+  R"cpp( // Tricky macros.
+#define MUL ) * (
+[[void func() [[{
+  // int var = (4 + 15 MUL 6 + 10);
+  int var = ([[4 + [[1^5 MUL]] 6 + 10);]]
+}
+   )cpp",
+  R"cpp( // Cursor inside a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HASH(2^3]] + 34]]);]]
+}
+   )cpp",
+  R"cpp( // Cursor on a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HA^SH(23);]]
+}
+   )cpp",
+  R"cpp( // Multiple declaration.
+[[void func() [[{
+  int var1, var^2]], var3;]]
+}
+   )cpp",
+  R"cpp( // Before comment.
+[[void func() [[{
+  int var1 = 1;
+  int var2 = var1]]^ /*some comment*/ + 41;]]
+}
+   )cpp",
+  // Empty file.
+  "^",
+  // FIXME: We should get the whole DeclStmt as a range.
+  R"cpp( // Single statement in TU.
+[[int v = [[1^00;
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.
+void func() {
+  int v = 100 + 100^;
+}
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor in between spaces.
+void func() {
+  int v = 100 + ^  100;
+}
+  )cpp",
+  // Structs.
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = AAA::BBB::c^cc]]();]]
+}
+  )cpp",
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = [[AA^A]]::]]BBB::]]ccc]]();]]
+}
+  )cpp",
+  R"cpp( // Inside struct.
+struct A { static int a(); };
+[[struct B { 
+  [[static int b() [[{
+[[return 1^1]] + 2;
+  }
+}]];
+  )cpp",
+  // Namespaces.
+  R"cpp( 
+[[namespace nsa { 
+  [[namespace nsb { 
+static int ccc();
+[[void func() [[{
+  // int x = nsa::nsb::ccc();
+  int x = nsa::nsb::cc^c]]();]]
+}
+  }]]
+}]]
+  )cpp",
+
+  };
+
+  for (const char *Test : Tests) {
+auto T = Annotations(Test);
+auto AST = TestTU::withCode(T.code()).build();
+EXPECT_THAT(llvm::cantFail(getSemanticRanges(AST, T.point())),
+ElementsAreArray(T.ranges()))
+<< Test;
+  }
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
\ No newline at end 

[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 220074.
usaxena95 added a comment.

Minor changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/SemanticSelection.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -0,0 +1,143 @@
+//===-- SemanticSelectionTests.cpp  *- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "Matchers.h"
+#include "Protocol.h"
+#include "SemanticSelection.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAreArray;
+
+TEST(SemanticSelection, All) {
+  const char *Tests[] = {
+  R"cpp( // Single statement in a function body.
+[[void func() [[{
+  int v = [[1^00;]]
+}
+  )cpp",
+  R"cpp( // Expression
+[[void func() [[{
+  int a = 1;
+  // int v = (10 + 2) * (a + a);
+  int v = (10^]] + 2]])]] * (a + a);]]
+}
+  )cpp",
+  R"cpp( // Function call.
+int add(int x, int y) { return x + y; }
+[[void callee() [[{
+  // int res = add(11, 22);
+  int res = [[add([[1^1]], 22);]]
+}
+  )cpp",
+  R"cpp( // Tricky macros.
+#define MUL ) * (
+[[void func() [[{
+  // int var = (4 + 15 MUL 6 + 10);
+  int var = ([[4 + [[1^5 MUL]] 6 + 10);]]
+}
+   )cpp",
+  R"cpp( // Cursor inside a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HASH(2^3]] + 34]]);]]
+}
+   )cpp",
+  R"cpp( // Cursor on a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HA^SH(23);]]
+}
+   )cpp",
+  R"cpp( // Multiple declaration.
+[[void func() [[{
+  int var1, var^2]], var3;]]
+}
+   )cpp",
+  R"cpp( // Before comment.
+[[void func() [[{
+  int var1 = 1;
+  int var2 = var1]]^ /*some comment*/ + 41;]]
+}
+   )cpp",
+  // Empty file.
+  "^",
+  // FIXME: We should get the whole DeclStmt as a range.
+  R"cpp( // Single statement in TU.
+[[int v = [[1^00;
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.
+void func() {
+  int v = 100 + 100^;
+}
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor in between spaces.
+void func() {
+  int v = 100 + ^  100;
+}
+  )cpp",
+  // Structs.
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = AAA::BBB::c^cc]]();]]
+}
+  )cpp",
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = [[AA^A]]::]]BBB::]]ccc]]();]]
+}
+  )cpp",
+  R"cpp( // Inside struct.
+struct A { static int a(); };
+[[struct B { 
+  [[static int b() [[{
+[[return 1^1]] + 2;
+  }
+}]];
+  )cpp",
+  // Namespaces.
+  R"cpp( 
+[[namespace nsa { 
+  [[namespace nsb { 
+static int ccc();
+[[void func() [[{
+  // int x = nsa::nsb::ccc();
+  int x = nsa::nsb::cc^c]]();]]
+}
+  }]]
+}]]
+  )cpp",
+
+  };
+
+  for (const char *Test : Tests) {
+auto T = Annotations(Test);
+auto AST = TestTU::withCode(T.code()).build();
+EXPECT_THAT(llvm::cantFail(getSemanticRanges(AST, T.point())),
+ElementsAreArray(T.ranges()))
+<< Test;
+  }
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
\ No newline at end of file
Index: 

[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:33
+  if (!Offset) {
+llvm::errs() << "Unable to convert postion to offset";
+return {};

sammccall wrote:
> we use log() or elog() for this, make sure you include Offset.takeError() for 
> the original message
> 
> Alternatively, you might consider returning Expected 
> so the error gets propagated to the client (as this really is a client error)
Returning Expected> now to propagate the error.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:39
+  SelectionTree ST(AST.getASTContext(), AST.getTokens(), *Offset);
+  for (const auto *Node = ST.commonAncestor(); Node != nullptr;
+   Node = Node->Parent) {

sammccall wrote:
> I think you probably want to bail out once you hit TranslationUnitDecl.
> 
> And maybe at namespace decls? Though I have often in my editor wondered 
> "what's the enclosing namespace"...
Bailing out once we hit the TUDecl. I am not sure about exiting on namespace 
decl. I am keeping this for now. We can remove this later if the need be.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:45
+}
+SourceLocation BeginLoc = SR.getBegin();
+SourceLocation EndLoc =

sammccall wrote:
> Ah, this will work if the start and the end of the AST node are written 
> directly in the file, and no macros are involved. But try 
> `toHalfOpenFileRange` in `SourceCode.h`, which should work in a lot more 
> cases.
> 
> In particular, this should work:
> ```
> #define INC(X) X + 1
> int a, b;
> int c = INC(a * b);
> ^
> ~
> ~
> ~~
> ~~
> ```
Interesting. This works fine in such cases. Thanks.
Though it works strangely with these ugly macros. I guess this is because it 
tries to follow the macro expansion.
```
#define MUL ) * (
int var = (4 + 15 MUL 6 + 10);
^
   ~~
   ~~
  ~~~
```



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:115
+[[void func() [[{
+  // int x = nsa::nsb::ccc();
+  int x = nsa::nsb::cc^c]]();]]

sammccall wrote:
> why commented out?
Since the statement contains many [[ and ]], I have added the actual statement 
in comments to make it more readable. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358



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

[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 220071.
usaxena95 marked 13 inline comments as done.
usaxena95 added a comment.

Addressed comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/SemanticSelection.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -0,0 +1,143 @@
+//===-- SemanticSelectionTests.cpp  *- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "Matchers.h"
+#include "Protocol.h"
+#include "SemanticSelection.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "llvm/Support/Error.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAreArray;
+
+TEST(SemanticSelection, All) {
+  const char *Tests[] = {
+  R"cpp( // Single statement in a function body.
+[[void func() [[{
+  int v = [[1^00;]]
+}
+  )cpp",
+  R"cpp( // Expression
+[[void func() [[{
+  int a = 1;
+  // int v = (10 + 2) * (a + a);
+  int v = (10^]] + 2]])]] * (a + a);]]
+}
+  )cpp",
+  R"cpp( // Function call.
+int add(int x, int y) { return x + y; }
+[[void callee() [[{
+  // int res = add(11, 22);
+  int res = [[add([[1^1]], 22);]]
+}
+  )cpp",
+  R"cpp( // Tricky macros.
+#define MUL ) * (
+[[void func() [[{
+  // int var = (4 + 15 MUL 6 + 10);
+  int var = ([[4 + [[1^5 MUL]] 6 + 10);]]
+}
+   )cpp",
+  R"cpp( // Cursor inside a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HASH(2^3]] + 34]]);]]
+}
+   )cpp",
+  R"cpp( // Cursor on a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = [[HA^SH(23);]]
+}
+   )cpp",
+  R"cpp( // Multiple declaration.
+[[void func() [[{
+  int var1, var^2]], var3;]]
+}
+   )cpp",
+  R"cpp( // Before comment.
+[[void func() [[{
+  int var1 = 1;
+  int var2 = var1]]^ /*some comment*/ + 41;]]
+}
+   )cpp",
+  // Empty file.
+  "^",
+  // FIXME: We should get the whole DeclStmt as a range.
+  R"cpp( // Single statement in TU.
+[[int v = [[1^00;
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.
+void func() {
+  int v = 100 + 100^;
+}
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor in between spaces.
+void func() {
+  int v = 100 + ^  100;
+}
+  )cpp",
+  // Structs.
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = AAA::BBB::c^cc]]();]]
+}
+  )cpp",
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = [[AA^A]]::]]BBB::]]ccc]]();]]
+}
+  )cpp",
+  R"cpp( // Inside struct.
+struct A { static int a(); };
+[[struct B { 
+  [[static int b() [[{
+[[return 1^1]] + 2;
+  }
+}]];
+  )cpp",
+  // Namespaces.
+  R"cpp( 
+[[namespace nsa { 
+  [[namespace nsb { 
+static int ccc();
+[[void func() [[{
+  // int x = nsa::nsb::ccc();
+  int x = nsa::nsb::cc^c]]();]]
+}
+  }]]
+}]]
+  )cpp",
+
+  };
+
+  for (const char *Test : Tests) {
+auto T = Annotations(Test);
+auto AST = TestTU::withCode(T.code()).build();
+EXPECT_THAT(llvm::cantFail(getSemanticRanges(AST, T.point())),
+ElementsAreArray(T.ranges()))
+<< Test;
+  }
+}
+} // namespace
+} // namespace clangd
+} // namespace 

[PATCH] D67358: [clangd] Implement semantic selections.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added a comment.

Nice! Particularly: great tests.

Only big thing is toHalfOpenFileRange should get you substantially better macro 
handling.




Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:18
+namespace {
+void addIfUnique(const Range , SemanticSelectionResult *Result) {
+  if (Result->Ranges.empty() || Result->Ranges.back() != R) {

nit: "unique" suggests this function checks against all the ranges in *Result 
(which it doesn't, and indeed doesn't need to)
I'd suggest renaming `addIfDistinct` and adding a comment that only consecutive 
ranges should be able to coincide



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:33
+  if (!Offset) {
+llvm::errs() << "Unable to convert postion to offset";
+return {};

we use log() or elog() for this, make sure you include Offset.takeError() for 
the original message

Alternatively, you might consider returning Expected 
so the error gets propagated to the client (as this really is a client error)



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:39
+  SelectionTree ST(AST.getASTContext(), AST.getTokens(), *Offset);
+  for (const auto *Node = ST.commonAncestor(); Node != nullptr;
+   Node = Node->Parent) {

I think you probably want to bail out once you hit TranslationUnitDecl.

And maybe at namespace decls? Though I have often in my editor wondered "what's 
the enclosing namespace"...



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:45
+}
+SourceLocation BeginLoc = SR.getBegin();
+SourceLocation EndLoc =

Ah, this will work if the start and the end of the AST node are written 
directly in the file, and no macros are involved. But try `toHalfOpenFileRange` 
in `SourceCode.h`, which should work in a lot more cases.

In particular, this should work:
```
#define INC(X) X + 1
int a, b;
int c = INC(a * b);
^
~
~
~~
~~
```



Comment at: clang-tools-extra/clangd/SemanticSelection.h:21
+
+struct SemanticSelectionResult {
+  // The list of interesting selections around the cursor. 

I'm not sure this struct pulls its weight, I'd suggest just returning 
vector here.
This isn't a stable API, we can change it if we need to add things.
Or is there something you have in mind?

Often we mirror the LSP here, but LSP is weird enough here (a linked list, 
seriously?) that I don't particularly think we should.



Comment at: clang-tools-extra/clangd/SemanticSelection.h:27
+// Returns the list of all interesting ranges around the Position \p Pos. 
+// Constructs the 
+// Any range in the result strictly contains all the previous ranges in the 
result.

incomplete sentence. It'd be nice to spell out on this line what an 
"interesting" range is (just one that corresponds to an AST node, right?)



Comment at: clang-tools-extra/clangd/SemanticSelection.h:28
+// Constructs the 
+// Any range in the result strictly contains all the previous ranges in the 
result.
+SemanticSelectionResult getSemanticRanges(ParsedAST , Position Pos);

somewhere you should mention that front() is the inner most range, and back() 
the outermost.



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:53
+   )cpp",
+  R"cpp( // Cursor inside a macro.
+#define HASH(x) ((x) % 10)

this is the case where `toHalfOpenFileName` should help



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:59
+   )cpp",
+  R"cpp( // Cursor on a macro.
+#define HASH(x) ((x) % 10)

interesting, this *might* be worth special handling, to select the whole macro 
expansion. Wonder if people will complain. Let's leave it for now for simplicity



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:82
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.

If you're interested, I think the fix here is in `pointBounds()` in 
selection.cpp.
Since selection-tree now treats semicolons as if they don't exist, we should 
probably handle them the same way we do whitespace, and look left instead of 
right.

(definitely a different patch though)



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:88
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.

oh, this is interesting. it's a side-effect of how selectiontree now ignores 
whitespace (and semicolons, and comments).

We convert a cursor into a size-one selection, and that used to always select 
something, 

[PATCH] D67358: [clangd] Implement semantic selections.

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 updated this revision to Diff 219386.
usaxena95 added a comment.

Create range only if it represents a valid file range.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/SemanticSelection.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -0,0 +1,133 @@
+//===-- SemanticSelectionTests.cpp  *- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "Matchers.h"
+#include "Protocol.h"
+#include "SemanticSelection.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+namespace clang {
+namespace clangd {
+namespace {
+using ::testing::ElementsAreArray;
+
+TEST(SemanticSelection, All) {
+  const char *Tests[] = {
+  R"cpp( // Single statement in a function body.
+[[void func() [[{
+  int v = [[1^00;]]
+}
+  )cpp",
+  R"cpp( // Expression
+[[void func() [[{
+  int a = 1;
+  // int v = (10 + 2) * (a + a);
+  int v = (10^]] + 2]])]] * (a + a);]]
+}
+  )cpp",
+  R"cpp( // Function call.
+int add(int x, int y) { return x + y; }
+[[void callee() [[{
+  // int res = add(11, 22);
+  int res = [[add([[1^1]], 22);]]
+}
+  )cpp",
+  R"cpp( // Tricky macros.
+#define MUL ) * (
+[[void func() [[{
+  // int var = (4 + 15 MUL 6 + 10);
+  int var = [[([[4 + [[1^5 MUL 6 + 10);]]
+}
+   )cpp",
+  R"cpp( // Cursor inside a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = HASH(2^3)]];]]
+}
+   )cpp",
+  R"cpp( // Cursor on a macro.
+#define HASH(x) ((x) % 10)
+[[void func() [[{
+  int a = HA^SH(23)]];]]
+}
+   )cpp",
+  R"cpp( // Multiple declaration.
+[[void func() [[{
+  int var1, var^2]], var3;]]
+}
+   )cpp",
+  R"cpp( // Before comment.
+[[void func() [[{
+  int var1 = 1;
+  int var2 = var1]]^ /*some comment*/ + 41;]]
+}
+   )cpp",
+  // Empty file.
+  "^",
+  // FIXME: We should get the whole DeclStmt as a range.
+  R"cpp( // Single statement in TU.
+[[int v = [[1^00;
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.
+void func() {
+  int v = 100 + 100^;
+}
+  )cpp",
+  // FIXME: No node found associated to the position.
+  R"cpp( // Cursor at end of VarDecl.
+void func() {
+  int v = 100 + ^  100;
+}
+  )cpp",
+  // Structs.
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = AAA::BBB::c^cc]]();]]
+}
+  )cpp",
+  R"cpp(
+struct AAA { struct BBB { static int ccc(); };};
+[[void func() [[{
+  // int x = AAA::BBB::ccc();
+  int x = [[AA^A]]::]]BBB::]]ccc]]();]]
+}
+  )cpp",
+  // Namespaces.
+  R"cpp( 
+[[namespace nsa { 
+  [[namespace nsb { 
+static int ccc();
+[[void func() [[{
+  // int x = nsa::nsb::ccc();
+  int x = nsa::nsb::cc^c]]();]]
+}
+  }]]
+}]]
+  )cpp",
+
+  };
+
+  for (const char *Test : Tests) {
+auto T = Annotations(Test);
+auto AST = TestTU::withCode(T.code()).build();
+auto Ranges = getSemanticRanges(AST, T.point()).Ranges;
+EXPECT_THAT(Ranges, ElementsAreArray(T.ranges())) << Test;
+  }
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
\ No newline at end of file
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -56,6 +56,7 @@