[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev 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 rG5ad6bbacf091: [clangd] Start using SyntaxTrees for folding 
ranges feature (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -271,29 +271,29 @@
 #endif
 }
 
-syntax::Leaf *syntax::Tree::findFirstLeaf() {
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findFirstLeaf() const {
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   return L;
-if (auto *L = cast(C)->findFirstLeaf())
+if (const auto *L = cast(C)->findFirstLeaf())
   return L;
   }
   return nullptr;
 }
 
-syntax::Leaf *syntax::Tree::findLastLeaf() {
-  syntax::Leaf *Last = nullptr;
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findLastLeaf() const {
+  const syntax::Leaf *Last = nullptr;
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   Last = L;
-else if (auto *L = cast(C)->findLastLeaf())
+else if (const auto *L = cast(C)->findLastLeaf())
   Last = L;
   }
   return Last;
 }
 
-syntax::Node *syntax::Tree::findChild(NodeRole R) {
-  for (auto *C = FirstChild; C; C = C->getNextSibling()) {
+const syntax::Node *syntax::Tree::findChild(NodeRole R) const {
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
 if (C->getRole() == R)
   return C;
   }
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -168,20 +168,24 @@
   Node *getFirstChild() { return FirstChild; }
   const Node *getFirstChild() const { return FirstChild; }
 
-  Leaf *findFirstLeaf();
-  const Leaf *findFirstLeaf() const {
-return const_cast(this)->findFirstLeaf();
+  const Leaf *findFirstLeaf() const;
+  Leaf *findFirstLeaf() {
+return const_cast(const_cast(this)->findFirstLeaf());
   }
 
-  Leaf *findLastLeaf();
-  const Leaf *findLastLeaf() const {
-return const_cast(this)->findLastLeaf();
+  const Leaf *findLastLeaf() const;
+  Leaf *findLastLeaf() {
+return const_cast(const_cast(this)->findLastLeaf());
+  }
+
+  /// Find the first node with a corresponding role.
+  const Node *findChild(NodeRole R) const;
+  Node *findChild(NodeRole R) {
+return const_cast(const_cast(this)->findChild(R));
   }
 
 protected:
   using Node::Node;
-  /// Find the first node with a corresponding role.
-  Node *findChild(NodeRole R);
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,61 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+#define FOO int foo() {\
+  int Variable = 42; \
+}
 
-[[void func() {
-  int v = 100;
-}]]
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+void func() {[[
+  int Variable = 100;
+
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 pri

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 301009.
kbobyrev added a comment.

Resolve merge conflict.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -271,29 +271,29 @@
 #endif
 }
 
-syntax::Leaf *syntax::Tree::findFirstLeaf() {
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findFirstLeaf() const {
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   return L;
-if (auto *L = cast(C)->findFirstLeaf())
+if (const auto *L = cast(C)->findFirstLeaf())
   return L;
   }
   return nullptr;
 }
 
-syntax::Leaf *syntax::Tree::findLastLeaf() {
-  syntax::Leaf *Last = nullptr;
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findLastLeaf() const {
+  const syntax::Leaf *Last = nullptr;
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   Last = L;
-else if (auto *L = cast(C)->findLastLeaf())
+else if (const auto *L = cast(C)->findLastLeaf())
   Last = L;
   }
   return Last;
 }
 
-syntax::Node *syntax::Tree::findChild(NodeRole R) {
-  for (auto *C = FirstChild; C; C = C->getNextSibling()) {
+const syntax::Node *syntax::Tree::findChild(NodeRole R) const {
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
 if (C->getRole() == R)
   return C;
   }
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -168,20 +168,24 @@
   Node *getFirstChild() { return FirstChild; }
   const Node *getFirstChild() const { return FirstChild; }
 
-  Leaf *findFirstLeaf();
-  const Leaf *findFirstLeaf() const {
-return const_cast(this)->findFirstLeaf();
+  const Leaf *findFirstLeaf() const;
+  Leaf *findFirstLeaf() {
+return const_cast(const_cast(this)->findFirstLeaf());
   }
 
-  Leaf *findLastLeaf();
-  const Leaf *findLastLeaf() const {
-return const_cast(this)->findLastLeaf();
+  const Leaf *findLastLeaf() const;
+  Leaf *findLastLeaf() {
+return const_cast(const_cast(this)->findLastLeaf());
+  }
+
+  /// Find the first node with a corresponding role.
+  const Node *findChild(NodeRole R) const;
+  Node *findChild(NodeRole R) {
+return const_cast(const_cast(this)->findChild(R));
   }
 
 protected:
   using Node::Node;
-  /// Find the first node with a corresponding role.
-  Node *findChild(NodeRole R);
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,61 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+#define FOO int foo() {\
+  int Variable = 42; \
+}
 
-[[void func() {
-  int v = 100;
-}]]
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+void func() {[[
+  int Variable = 100;
+
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]}
 
-  [[void getFooBar() {

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 301008.
kbobyrev added a comment.

Address post-LGTM comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -271,29 +271,29 @@
 #endif
 }
 
-syntax::Leaf *syntax::Tree::findFirstLeaf() {
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findFirstLeaf() const {
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   return L;
-if (auto *L = cast(C)->findFirstLeaf())
+if (const auto *L = cast(C)->findFirstLeaf())
   return L;
   }
   return nullptr;
 }
 
-syntax::Leaf *syntax::Tree::findLastLeaf() {
-  syntax::Leaf *Last = nullptr;
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findLastLeaf() const {
+  const syntax::Leaf *Last = nullptr;
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   Last = L;
-else if (auto *L = cast(C)->findLastLeaf())
+else if (const auto *L = cast(C)->findLastLeaf())
   Last = L;
   }
   return Last;
 }
 
-syntax::Node *syntax::Tree::findChild(NodeRole R) {
-  for (auto *C = FirstChild; C; C = C->getNextSibling()) {
+const syntax::Node *syntax::Tree::findChild(NodeRole R) const {
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
 if (C->getRole() == R)
   return C;
   }
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -168,20 +168,21 @@
   Node *getFirstChild() { return FirstChild; }
   const Node *getFirstChild() const { return FirstChild; }
 
-  Leaf *findFirstLeaf();
-  const Leaf *findFirstLeaf() const {
-return const_cast(this)->findFirstLeaf();
+  const Leaf *findFirstLeaf() const;
+  Leaf *findFirstLeaf() {
+return const_cast(const_cast(this)->findFirstLeaf());
   }
 
-  Leaf *findLastLeaf();
-  const Leaf *findLastLeaf() const {
-return const_cast(this)->findLastLeaf();
+  const Leaf *findLastLeaf() const;
+  Leaf *findLastLeaf() {
+return const_cast(const_cast(this)->findLastLeaf());
   }
 
-protected:
-  using Node::Node;
   /// Find the first node with a corresponding role.
-  Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const;
+  Node *findChild(NodeRole R) {
+return const_cast(const_cast(this)->findChild(R));
+  }
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,61 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+#define FOO int foo() {\
+  int Variable = 42; \
+}
 
-[[void func() {
-  int v = 100;
-}]]
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+void func() {[[
+  int Variable = 100;
+
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]}
 
-  [[void getFooBar() { }]]
-}]];
+  // Braces are located 

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:42
+
+llvm::Optional toFoldingRange(LocInfo Begin, LocInfo End,
+const SourceManager &SM) {

just use SourceLocations or SourceRange here? We have the SourceManager to 
decompose them anyway.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:60
+   *RBrace = cast_or_null(
+   Stmt->findChild(syntax::NodeRole::CloseParen));
+if (!LBrace || !RBrace)

sammccall wrote:
> strictly this should probably be findLastChild both semantically and for 
> performance... but that doesn't exist, because it's a single-linked list for 
> now.
> 
> Not a big deal in practice, but we should fix this (and add STL iterators!)
Added a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 300479.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.

Decompose locations and add checks for FileID == MainFileID.

Also, rebase on top of master.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -271,29 +271,29 @@
 #endif
 }
 
-syntax::Leaf *syntax::Tree::findFirstLeaf() {
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findFirstLeaf() const {
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   return L;
-if (auto *L = cast(C)->findFirstLeaf())
+if (const auto *L = cast(C)->findFirstLeaf())
   return L;
   }
   return nullptr;
 }
 
-syntax::Leaf *syntax::Tree::findLastLeaf() {
-  syntax::Leaf *Last = nullptr;
-  for (auto *C = getFirstChild(); C; C = C->getNextSibling()) {
-if (auto *L = dyn_cast(C))
+const syntax::Leaf *syntax::Tree::findLastLeaf() const {
+  const syntax::Leaf *Last = nullptr;
+  for (const auto *C = getFirstChild(); C; C = C->getNextSibling()) {
+if (const auto *L = dyn_cast(C))
   Last = L;
-else if (auto *L = cast(C)->findLastLeaf())
+else if (const auto *L = cast(C)->findLastLeaf())
   Last = L;
   }
   return Last;
 }
 
-syntax::Node *syntax::Tree::findChild(NodeRole R) {
-  for (auto *C = FirstChild; C; C = C->getNextSibling()) {
+const syntax::Node *syntax::Tree::findChild(NodeRole R) const {
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
 if (C->getRole() == R)
   return C;
   }
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -159,19 +159,21 @@
   Node *getFirstChild() { return FirstChild; }
   const Node *getFirstChild() const { return FirstChild; }
 
-  Leaf *findFirstLeaf();
-  const Leaf *findFirstLeaf() const {
-return const_cast(this)->findFirstLeaf();
+  const Leaf *findFirstLeaf() const;
+  Leaf *findFirstLeaf() {
+return const_cast(const_cast(this)->findFirstLeaf());
   }
 
-  Leaf *findLastLeaf();
-  const Leaf *findLastLeaf() const {
-return const_cast(this)->findLastLeaf();
+  const Leaf *findLastLeaf() const;
+  Leaf *findLastLeaf() {
+return const_cast(const_cast(this)->findLastLeaf());
   }
 
-protected:
   /// Find the first node with a corresponding role.
-  Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const;
+  Node *findChild(NodeRole R) {
+return const_cast(const_cast(this)->findChild(R));
+  }
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,61 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+#define FOO int foo() {\
+  int Variable = 42; \
+}
 
-[[void func() {
-  int v = 100;
-}]]
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+void func() {[[
+  int Variable = 100;
+
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:40
 
-// Recursively collects FoldingRange from a symbol and its children.
-void collectFoldingRanges(DocumentSymbol Symbol,
-  std::vector &Result) {
+llvm::Optional toFoldingRange(SourceRange SR,
+const SourceManager &SM) {

Here you're looking at positions but ignoring file ID, which is always a reason 
to be cautious...

We've dropped macro IDs, and we're traversing only the syntax tree for this 
file, but we could still end up in the wrong file:
```
void foo() {
  #include "SomeTablegen.inc"
}
```

We need to verify both start/end are in the main file.

I'd suggest passing in the main FileID as a param here, and decomposing the 
start/end locations into (FileID, Offset), and bailing out if either FID 
doesn't match.
This subsumes the macro check (since macro expansions have their own FileIDs) 
though you might want to keep it to be explicit, as we likely want to support 
some cases later.

As a bonus, then you get to use SourceManager::get{Line,Column}Number(FileID, 
Offset) instead of the "spelling" variants that are slightly confusing as we've 
already established we have a file location already.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:60
+   *RBrace = cast_or_null(
+   Stmt->findChild(syntax::NodeRole::CloseParen));
+if (!LBrace || !RBrace)

strictly this should probably be findLastChild both semantically and for 
performance... but that doesn't exist, because it's a single-linked list for 
now.

Not a big deal in practice, but we should fix this (and add STL iterators!)



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:158
+  const auto *SyntaxTree =
+  syntax::buildSyntaxTree(A, 
*AST.getASTContext().getTranslationUnitDecl());
+  return collectFoldingRanges(SyntaxTree, AST.getSourceManager());

this actually does the right thing (traverses the ASTContext's registered roots 
rather than the TUDecl itself), but... what a misleading API. We should fix 
that to take ASTContext instead...

(Not in this patch, maybe a followup?)



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:175
+  const Node *findChild(NodeRole R) const {
+return const_cast(this)->findChild(R);
+  }

I think it's more conventional to implement the const variant and use 
const_cast for the non-const variant, so the compiler will verify that the 
function implementation is const, and the cast is just adding "I know if I'm 
not const then the returned object isn't either".

By implementing the *non-const* variant, the compiler doesn't help you verify 
any part of the const contract.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:172-174
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const;

eduucaldas wrote:
> I think that makes sense, since all the functions used by `findChild` are 
> public anyways.
> 
> WDYT Dmitri? Now that the API is being used I realize its surface ^^. Perhaps 
> we should pour some thought into that in the future :) 
> 
I think it makes sense. Const-correctness often creates this type of API 
duplication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 299620.
kbobyrev added a comment.

Fix a typo and remove invalid const-ness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h

Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -169,9 +169,11 @@
 return const_cast(this)->findLastLeaf();
   }
 
-protected:
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const {
+return const_cast(this)->findChild(R);
+  }
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,59 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+void func() {[[
+  int Variable = 100;
 
-[[void func() {
-  int v = 100;
-}]]
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+#define FOO int foo() {}
+
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]}
 
-  [[void getFooBar() { }]]
-}]];
+  // Braces are located at the same line: no folding range here.
+  void getFooBar() { }
+};
   )cpp",
   };
   for (const char *Test : Tests) {
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
+
 #include "SemanticSelection.h"
 #include "FindSymbols.h"
 #include "ParsedAST.h"
@@ -13,8 +14,16 @@
 #include "SourceCode.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
+#include "clang/Tooling/Syntax/BuildTree.h"
+#include "clang/Tooling/Syntax/Nodes.h"
+#include "clang/Tooling/Syntax/Tree.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -28,17 +37,59 @@
   }
 }
 
-// Recursively collects FoldingRange from a symbol and its children.
-void collectFoldingRanges(DocumentSymbol Symbol,
-  std::vector &Result) {
+llvm::Optional toFoldingRange(SourceRange SR,
+const SourceManager &SM) {
+  // Don't produce folding ranges when SourceRange either of bounds is coming
+  // from macros.
+  if (SR.getBegin().isMacroID() || SR.getEnd().isMacroID())
+return llvm::None;
   FoldingRange Range;
-  Range.startLine = Symbol.range.start.line;
-  Range.startCharacter = Symbol.range.start.character;
-  Range.endLine = Symbol.range.end.line;
-  Range.endCharacter = Symbol.range.end.character;
-  Result.push_back(Range);
-  for (const auto &Child : Symbol.children)
-collectFoldingRanges(Child, Result);
+  Range.startCharacter = SM.getSpellingColumnNumber(SR.getBegin()) - 1;
+  Range.startLine = SM.getSpellingLineNumber(SR.getBegin()) - 1;
+  Range.endCharacter = SM.getSpellingColumnNumber(SR.getEnd()) - 1;
+  Range.endLine = SM.getSpellingLineNumber(SR.getEnd()) - 1

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 299619.
kbobyrev marked an inline comment as done.
kbobyrev added a comment.

Prevent code duplication for const-nonconst versions of the same function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -293,7 +293,7 @@
 }
 
 syntax::Node *syntax::Tree::findChild(NodeRole R) {
-  for (auto *C = FirstChild; C; C = C->getNextSibling()) {
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
 if (C->getRole() == R)
   return C;
   }
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -169,9 +169,11 @@
 return const_cast(this)->findLastLeaf();
   }
 
-protected:
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const {
+return const_cast(this)->findChild(R);
+  }
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,59 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+void func() {[[
+  int Variable = 100;
 
-[[void func() {
-  int v = 100;
-}]]
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+#define FOO int foo() {}
+
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]}
 
-  [[void getFooBar() { }]]
-}]];
+  // Braces are located at the same line: no folding range here.
+  void getFooBar() { }
+};
   )cpp",
   };
   for (const char *Test : Tests) {
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
+
 #include "SemanticSelection.h"
 #include "FindSymbols.h"
 #include "ParsedAST.h"
@@ -13,8 +14,16 @@
 #include "SourceCode.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
+#include "clang/Tooling/Syntax/BuildTree.h"
+#include "clang/Tooling/Syntax/Nodes.h"
+#include "clang/Tooling/Syntax/Tree.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -28,17 +37,59 @@
   }
 }
 
-// Recursively collects FoldingRange from a symbol and its children.
-void collectFoldingRanges(DocumentSymbol Symbol,
-  std::vector &Result) {
+llvm::Optional toFoldingRange(SourceRange SR,
+const SourceManager &SM) {
+  // Don't produce folding ranges when SourceRange either of bounds is coming
+  // from macros.
+  if (SR.getBegin().isMacroID() || SR.getEnd().isMacroID())
+return llvm::None;
   FoldingRange Range;
-  Range.startLine = Symbol.

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.



Comment at: clang/include/clang/Tooling/Syntax/Tree.h:172-174
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const;

I think that makes sense, since all the functions used by `findChild` are 
public anyways.

WDYT Dmitri? Now that the API is being used I realize its surface ^^. Perhaps 
we should pour some thought into that in the future :) 




Comment at: clang/lib/Tooling/Syntax/Tree.cpp:304-308
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
+if (C->getRole() == R)
+  return C;
+  }
+  return nullptr;

Similarly to the const version of `findFirstLeaf`. I think this should work :)

Also you could put the definition in `clang/Tooling/Syntax/Tree.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > eduucaldas wrote:
> > > > Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax 
> > > > constructs usually have nice classes with accessors.
> > > > 
> > > > For instance `CompoundStatement` has the accessors `getLbrace` and 
> > > > `getRbrace` that seem to be exactly what you want.
> > > > 
> > > > However these might not give exactly the first leaf and last leaf in 
> > > > the case of syntactically incorrect code.
> > > I think we should treat all bracket-like things generically. Today this 
> > > is just CompoundStmt, but we want to handle init lists, function calls, 
> > > parens around for-loop conditions, template parameter and arg lists etc 
> > > in the same way.
> > > 
> > > This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are 
> > > generic - we can have one set of logic to handle all of these. (No 
> > > opinion on whether that should live here or in the syntax trees library, 
> > > but putting it here for now seems fine).
> > > 
> > > So in the end I think checking the class name and then grabbing the 
> > > braces by role (not kind) is the right thing here.
> > > We definitely want to avoid asserting that the code looks the way we 
> > > expect though.
> > > So in the end I think checking the class name and then grabbing the 
> > > braces by role (not kind) is the right thing here.
> > > We definitely want to avoid asserting that the code looks the way we 
> > > expect though.
> > 
> > Can you elaborate a bit on how this would work? Is your proposal to iterate 
> > through `CompoundStatement` first-level children and grab the `OpenParen` + 
> > `CloseParen` roles?
> Exactly. And bail out if both don't exist.
> 
> And this can be done on Tree, so it's trivial to add support for function 
> calls etc (but feel free to keep the scope small)
I very much agree on all the points you made Sam.

I think the logic to get ranges from node roles should eventually live in the 
syntax trees library. We might want to hide some of these lower-level functions 
in the future, so it would be better to not rely on them. But it's no harm for 
now :).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 299449.
kbobyrev added a comment.

Resolve comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -300,6 +300,14 @@
   return nullptr;
 }
 
+const syntax::Node *syntax::Tree::findChild(NodeRole R) const {
+  for (const auto *C = FirstChild; C; C = C->getNextSibling()) {
+if (C->getRole() == R)
+  return C;
+  }
+  return nullptr;
+}
+
 bool syntax::List::classof(const syntax::Node *N) {
   switch (N->getKind()) {
   case syntax::NodeKind::NestedNameSpecifier:
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -169,9 +169,9 @@
 return const_cast(this)->findLastLeaf();
   }
 
-protected:
   /// Find the first node with a corresponding role.
   Node *findChild(NodeRole R);
+  const Node *findChild(NodeRole R) const;
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,59 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+void func() {[[
+  int Variable = 100;
 
-[[void func() {
-  int v = 100;
-}]]
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+#define FOO int foo() {}
+
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]}
 
-  [[void getFooBar() { }]]
-}]];
+  // Braces are located at the same line: no folding range here.
+  void getFooBar() { }
+};
   )cpp",
   };
   for (const char *Test : Tests) {
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
+
 #include "SemanticSelection.h"
 #include "FindSymbols.h"
 #include "ParsedAST.h"
@@ -13,8 +14,16 @@
 #include "SourceCode.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
+#include "clang/Tooling/Syntax/BuildTree.h"
+#include "clang/Tooling/Syntax/Nodes.h"
+#include "clang/Tooling/Syntax/Tree.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -28,17 +37,59 @@
   }
 }
 
-// Recursively collects FoldingRange from a symbol and its children.
-void collectFoldingRanges(DocumentSymbol Symbol,
-  std::vector &Result) {
+llvm::Optional toFoldingRange(SourceRange SR,
+const SourceManager &SM) {
+  // Don't produce folding ranges when SourceRange either of bounds is coming
+  // from macros.
+  if (SR.getBegin().isMacroID() || SR.getEnd().isMacroID())
+return llvm::None;
   FoldingRange Range;
-  Range.startLine = Symbol.range.start.line;
-  Range.s

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

kbobyrev wrote:
> sammccall wrote:
> > eduucaldas wrote:
> > > Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax 
> > > constructs usually have nice classes with accessors.
> > > 
> > > For instance `CompoundStatement` has the accessors `getLbrace` and 
> > > `getRbrace` that seem to be exactly what you want.
> > > 
> > > However these might not give exactly the first leaf and last leaf in the 
> > > case of syntactically incorrect code.
> > I think we should treat all bracket-like things generically. Today this is 
> > just CompoundStmt, but we want to handle init lists, function calls, parens 
> > around for-loop conditions, template parameter and arg lists etc in the 
> > same way.
> > 
> > This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are generic 
> > - we can have one set of logic to handle all of these. (No opinion on 
> > whether that should live here or in the syntax trees library, but putting 
> > it here for now seems fine).
> > 
> > So in the end I think checking the class name and then grabbing the braces 
> > by role (not kind) is the right thing here.
> > We definitely want to avoid asserting that the code looks the way we expect 
> > though.
> > So in the end I think checking the class name and then grabbing the braces 
> > by role (not kind) is the right thing here.
> > We definitely want to avoid asserting that the code looks the way we expect 
> > though.
> 
> Can you elaborate a bit on how this would work? Is your proposal to iterate 
> through `CompoundStatement` first-level children and grab the `OpenParen` + 
> `CloseParen` roles?
Exactly. And bail out if both don't exist.

And this can be done on Tree, so it's trivial to add support for function calls 
etc (but feel free to keep the scope small)



Comment at: clang/include/clang/Tooling/Syntax/Nodes.h:771
   }
-  Leaf *getLbrace();
+  Leaf *getLbrace() const;
   /// FIXME: use custom iterator instead of 'vector'.

This doesn't look right: now a const method grants mutable access to the child. 
I think we need both overloads :-(

(Fortunately this is to be tablegen'd one day...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

sammccall wrote:
> eduucaldas wrote:
> > Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax 
> > constructs usually have nice classes with accessors.
> > 
> > For instance `CompoundStatement` has the accessors `getLbrace` and 
> > `getRbrace` that seem to be exactly what you want.
> > 
> > However these might not give exactly the first leaf and last leaf in the 
> > case of syntactically incorrect code.
> I think we should treat all bracket-like things generically. Today this is 
> just CompoundStmt, but we want to handle init lists, function calls, parens 
> around for-loop conditions, template parameter and arg lists etc in the same 
> way.
> 
> This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are generic - 
> we can have one set of logic to handle all of these. (No opinion on whether 
> that should live here or in the syntax trees library, but putting it here for 
> now seems fine).
> 
> So in the end I think checking the class name and then grabbing the braces by 
> role (not kind) is the right thing here.
> We definitely want to avoid asserting that the code looks the way we expect 
> though.
> So in the end I think checking the class name and then grabbing the braces by 
> role (not kind) is the right thing here.
> We definitely want to avoid asserting that the code looks the way we expect 
> though.

Can you elaborate a bit on how this would work? Is your proposal to iterate 
through `CompoundStatement` first-level children and grab the `OpenParen` + 
`CloseParen` roles?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 299438.
kbobyrev marked 9 inline comments as done.
kbobyrev added a comment.

Resolve review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

Files:
  clang-tools-extra/clangd/SemanticSelection.cpp
  clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
  clang/include/clang/Tooling/Syntax/Nodes.h
  clang/include/clang/Tooling/Syntax/Tree.h
  clang/lib/Tooling/Syntax/Nodes.cpp
  clang/lib/Tooling/Syntax/Tree.cpp

Index: clang/lib/Tooling/Syntax/Tree.cpp
===
--- clang/lib/Tooling/Syntax/Tree.cpp
+++ clang/lib/Tooling/Syntax/Tree.cpp
@@ -292,7 +292,7 @@
   return Last;
 }
 
-syntax::Node *syntax::Tree::findChild(NodeRole R) {
+syntax::Node *syntax::Tree::findChild(NodeRole R) const {
   for (auto *C = FirstChild; C; C = C->getNextSibling()) {
 if (C->getRole() == R)
   return C;
Index: clang/lib/Tooling/Syntax/Nodes.cpp
===
--- clang/lib/Tooling/Syntax/Nodes.cpp
+++ clang/lib/Tooling/Syntax/Nodes.cpp
@@ -522,7 +522,7 @@
   findChild(syntax::NodeRole::Expression));
 }
 
-syntax::Leaf *syntax::CompoundStatement::getLbrace() {
+syntax::Leaf *syntax::CompoundStatement::getLbrace() const {
   return cast_or_null(findChild(syntax::NodeRole::OpenParen));
 }
 
@@ -535,7 +535,7 @@
   return Children;
 }
 
-syntax::Leaf *syntax::CompoundStatement::getRbrace() {
+syntax::Leaf *syntax::CompoundStatement::getRbrace() const {
   return cast_or_null(findChild(syntax::NodeRole::CloseParen));
 }
 
Index: clang/include/clang/Tooling/Syntax/Tree.h
===
--- clang/include/clang/Tooling/Syntax/Tree.h
+++ clang/include/clang/Tooling/Syntax/Tree.h
@@ -171,7 +171,7 @@
 
 protected:
   /// Find the first node with a corresponding role.
-  Node *findChild(NodeRole R);
+  Node *findChild(NodeRole R) const;
 
 private:
   /// Prepend \p Child to the list of children and and sets the parent pointer.
Index: clang/include/clang/Tooling/Syntax/Nodes.h
===
--- clang/include/clang/Tooling/Syntax/Nodes.h
+++ clang/include/clang/Tooling/Syntax/Nodes.h
@@ -768,10 +768,10 @@
   static bool classof(const Node *N) {
 return N->getKind() == NodeKind::CompoundStatement;
   }
-  Leaf *getLbrace();
+  Leaf *getLbrace() const;
   /// FIXME: use custom iterator instead of 'vector'.
   std::vector getStatements();
-  Leaf *getRbrace();
+  Leaf *getRbrace() const;
 };
 
 /// A declaration that can appear at the top-level. Note that this does *not*
Index: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
===
--- clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
@@ -203,26 +203,59 @@
 TEST(FoldingRanges, All) {
   const char *Tests[] = {
   R"cpp(
-[[int global_variable]];
+void func() {[[
+  int Variable = 100;
 
-[[void func() {
-  int v = 100;
-}]]
+  if (Variable > 5) {[[
+Variable += 42;
+  ]]} else if (Variable++)
+++Variable;
+  else {[[
+Variable--;
+  ]]}
+
+  // Do not generate FoldingRange for empty CompoundStmts.
+  for (;;) {}
+
+  // If there are newlines between {}, we should generate one.
+  for (;;) {[[
+
+  ]]}
+]]}
   )cpp",
   R"cpp(
-[[class Foo {
+#define FOO int foo() {}
+
+// Do not generate folding range for braces within macro expansion.
+FOO
+
+// Do not generate folding range within macro arguments.
+#define FUNCTOR(functor) functor
+void func() {[[
+  FUNCTOR([](){});
+]]}
+
+// Do not generate folding range with a brace coming from macro.
+#define LBRACE {
+void bar() LBRACE
+  int X = 42;
+}
+  )cpp",
+  R"cpp(
+class Foo {
 public:
-  [[Foo() {
+  Foo() {[[
 int X = 1;
-  }]]
+  ]]}
 
 private:
-  [[int getBar() {
+  int getBar() {[[
 return 42;
-  }]]
+  ]]}
 
-  [[void getFooBar() { }]]
-}]];
+  // Braces are located at the same line: no folding range here.
+  void getFooBar() { }
+};
   )cpp",
   };
   for (const char *Test : Tests) {
Index: clang-tools-extra/clangd/SemanticSelection.cpp
===
--- clang-tools-extra/clangd/SemanticSelection.cpp
+++ clang-tools-extra/clangd/SemanticSelection.cpp
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:39
   FoldingRange Range;
-  Range.startLine = Symbol.range.start.line;
-  Range.startCharacter = Symbol.range.start.character;
-  Range.endLine = Symbol.range.end.line;
-  Range.endCharacter = Symbol.range.end.character;
-  Result.push_back(Range);
-  for (const auto &Child : Symbol.children)
-collectFoldingRanges(Child, Result);
+  Range.startCharacter = SM.getSpellingColumnNumber(SR.getBegin()) - 1;
+  Range.startLine = SM.getSpellingLineNumber(SR.getBegin()) - 1;

Have you considered how you want macro-expanded code to work?

As written, this code can produce ranges inside macro definitions, invalid 
ranges (if { and } aren't from the same macro expansion) or even crash 
(FirstToken->endLocation() may be an invalid one-past-the-end location).

My suggestion would be to have this return optional and simply 
bail out if the location are not file locations for now. (Later we can deal 
with macro args in some cases)



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

eduucaldas wrote:
> Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax 
> constructs usually have nice classes with accessors.
> 
> For instance `CompoundStatement` has the accessors `getLbrace` and 
> `getRbrace` that seem to be exactly what you want.
> 
> However these might not give exactly the first leaf and last leaf in the case 
> of syntactically incorrect code.
I think we should treat all bracket-like things generically. Today this is just 
CompoundStmt, but we want to handle init lists, function calls, parens around 
for-loop conditions, template parameter and arg lists etc in the same way.

This sort of use is why the `OpenParen`/`CloseParen` NodeRoles are generic - we 
can have one set of logic to handle all of these. (No opinion on whether that 
should live here or in the syntax trees library, but putting it here for now 
seems fine).

So in the end I think checking the class name and then grabbing the braces by 
role (not kind) is the right thing here.
We definitely want to avoid asserting that the code looks the way we expect 
though.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:56
+assert(LastToken->kind() == tok::TokenKind::r_brace);
+const SourceRange SR(FirstToken->endLocation(), LastToken->location());
+FoldingRange Range = constructFoldingRange(SR, SM);

this is worthy of a comment (fold the entire range inside the brackets, 
including whitespace)



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:60
+// nodes and newlines.
+if (Tree->findFirstLeaf()->getNextSibling() != Tree->findLastLeaf() ||
+Range.startLine != Range.endLine)

until we support FoldingRangeClientCapabilities, should we just have the line 
check?



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:206
   R"cpp(
-[[int global_variable]];
+void func() {[[
+  int Variable = 100;

This is too many test cases for straightforward compoundstmt, I think. We have 
only once type of node handled for now, but we can't afford to write and 
maintain this many tests once we have lots.

I think a single if{}-elseif(nobraces)-else{} along with the existing function 
examples is probably enough. (Function examples show missing range for empty 
body, though it could use a comment)

We definitely need cases for macros:
 - entire {} within one macro arg
 - entire {} in the macro body
 - some combination (e.g. { in macro body, } outside the macro)
(we won't need these for every node type, assuming we have some consistent 
handling)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:37
 
-// Recursively collects FoldingRange from a symbol and its children.
-void collectFoldingRanges(DocumentSymbol Symbol,
-  std::vector &Result) {
+FoldingRange constructFoldingRange(SourceRange SR, const SourceManager &SM) {
   FoldingRange Range;

WDYT about "makeFoldingRange" or even "toFoldingRange" to emphasize it is a 
factory / conversion function (no actual computation)?



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:47
+// Traverse the tree and collect folding ranges along the way.
+void collectRanges(const syntax::Node *Node, const SourceManager &SM,
+   std::vector &Ranges) {

"collectFoldingRanges" was a better name I think.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:48
+void collectRanges(const syntax::Node *Node, const SourceManager &SM,
+   std::vector &Ranges) {
+  if (Node->getKind() == syntax::NodeKind::CompoundStatement) {

Why not return the vector?



Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:246
+
+  // However, if there are newlines between {}, we will still generate
+  // one.

"Will" makes it sound like it is not intentional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Eduardo Caldas via Phabricator via cfe-commits]

eduucaldas added inline comments.



Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:49-51
+  if (Node->getKind() == syntax::NodeKind::CompoundStatement) {
+const auto *Tree = dyn_cast(Node);
+assert(Tree);





Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:52-55
+const syntax::Token *FirstToken = Tree->findFirstLeaf()->getToken(),
+*LastToken = Tree->findLastLeaf()->getToken();
+assert(FirstToken->kind() == tok::TokenKind::l_brace);
+assert(LastToken->kind() == tok::TokenKind::r_brace);

Take a look at `clang/include/clang/Tooling/Syntax/Nodes.h`, syntax constructs 
usually have nice classes with accessors.

For instance `CompoundStatement` has the accessors `getLbrace` and `getRbrace` 
that seem to be exactly what you want.

However these might not give exactly the first leaf and last leaf in the case 
of syntactically incorrect code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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

[PATCH] D88553: [clangd] Start using SyntaxTrees for folding ranges feature

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

@sammccall I've reduced the patch to the bare minimum (compound statements) as 
I've had some issues with couple of other kinds of folding ranges and I'll be 
adding support for the node kinds one by one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88553

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