[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<FoldingRange> &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<FoldingRange> &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<FoldingRange> &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