Add foldings for multi-line comment.

[hokein]

clang-tools-extra/clangd/SemanticSelection.cpp

176–177

nit: update the fixme, comment is supported now.

239

we can save some cost if we first get the final End comment, and calculate the offset from it.

243

since LSP defines 3 kinds (region, comment, import), I'd suggest defining these string literals in the Protocol.h, rather than using the magic string here.

clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

351

For this case, my personal preference would be to have 3 different foldings, rather than a union one.

If you think about cases like below, it makes more sense to have individual folding per comment.

/* comment 1
*/

/* comment 2
*/

/* comment 1
*/

// comment 2

[usaxena95]

Addressed comments.

clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

351

In the cases mentioned, I think these comments would be about the same context. If I want to fold (possibly long) comments with interleaved new lines, I would be interested in folding all the related comments to look at the code clearly.

[usaxena95]

Do not fill Kind in JSON when it is unspecified.

[hokein]

clang-tools-extra/clangd/Protocol.h

1779

sorry for not being clear on my previous comment, I think the current string kind; is good, and it aligns with the LSP one.

what I suggest was adding string literals to FoldingRange, something like

struct FoldingRange {
  const static llvm::StringLiteral REGION_KIND;
  const static llvm::StringLiteral COMMENT_KIND;
  const static llvm::StringLiteral IMPORT_KIND;
  
  // other fields keep as-is.
  ...
}

we're diverging from the LSP structure.

clang-tools-extra/clangd/SemanticSelection.cpp

239

this seems incorrect -- the LastComment->Line == T->Line + 1 condition is not true initially (because LastComment == T), so we will never run into this loop, then I'm confused, why the unittest are passed (premerge test is green).

clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

351

In the cases mentioned, I think these comments would be about the same context.

I'm not sure this is true. I could image a case like below, these comments are not about the same context. I think for cases where these comments are about the same context, they are generally separated by // <blankline> rather than <blankline>.

btw, Kirill has a patch (https://reviews.llvm.org/D83914) for proposed folding behaviors, see the comments part (which seems reasonable to me).

//===-- This is a file comment ------------------------------------------===//
//
// ...
//
//===----------------------------------------------------------------------===//


// This is a doc comment of `foo` class
class Foo {};

[kadircet]

clang-tools-extra/clangd/Protocol.h

1779

+1, we already have the same pattern for code action kinds, i was also referring to this in our offline discussion.

[usaxena95]

Addressed comments.

clang-tools-extra/clangd/SemanticSelection.cpp

239

I think I missed to update the patch last time I fixed this.

[usaxena95]

Remove unintended changes.

[hokein]

clang-tools-extra/clangd/SemanticSelection.cpp

177–179

nit: there is an extra space (I'd rather just use FIXME:)

224–226

nit: add a comment explaining the +1 offset (skipping the bracket).

239

nit: the <= seems to do much --I think we care about = LastCommentEnd.line and = LastCommentEnd.line + 1 cases, < LastCommentEnd.line should be impossible, right?

clang-tools-extra/clangd/Protocol.h

bool fromJSON(const llvm::json::Value &, FoldingRangeParams &,
               llvm::json::Path);
 
 /// Stores information about a region of code that can be folded.
 struct FoldingRange {
   unsigned startLine = 0;
   unsigned startCharacter;
   unsigned endLine = 0;
   unsigned endCharacter;
+
+  const static llvm::StringLiteral REGION_KIND;
+  const static llvm::StringLiteral COMMENT_KIND;
+  const static llvm::StringLiteral IMPORT_KIND;
   std::string kind;
 };
 llvm::json::Value toJSON(const FoldingRange &Range);
 
 /// Keys starting with an underscore(_) represent leaves, e.g. _total or _self
 /// for memory usage of whole subtree or only that specific node in bytes. All
 /// other keys represents children. An example:
 ///   {
 ///     "_self": 0,

clang-tools-extra/clangd/Protocol.cpp

 }
 
 bool fromJSON(const llvm::json::Value &Params, FoldingRangeParams &R,
               llvm::json::Path P) {
   llvm::json::ObjectMapper O(Params, P);
   return O && O.map("textDocument", R.textDocument);
 }
 
+const llvm::StringLiteral FoldingRange::REGION_KIND = "region";
+const llvm::StringLiteral FoldingRange::COMMENT_KIND = "comment";
+const llvm::StringLiteral FoldingRange::IMPORT_KIND = "import";
+
 llvm::json::Value toJSON(const FoldingRange &Range) {
   llvm::json::Object Result{
       {"startLine", Range.startLine},
       {"endLine", Range.endLine},
   };
   if (Range.startCharacter)
     Result["startCharacter"] = Range.startCharacter;
   if (Range.endCharacter)
     Result["endCharacter"] = Range.endCharacter;
-  if (!Range.kind.empty())
+  if (!Range.kind.empty()) {
     Result["kind"] = Range.kind;
+  }
   return Result;
 }
 
 llvm::json::Value toJSON(const MemoryTree &MT) {
   llvm::json::Object Out;
   int64_t Total = MT.self();
   Out["_self"] = Total;
   for (const auto &Entry : MT.children()) {

clang-tools-extra/clangd/SemanticSelection.cpp

 #include "clang/AST/DeclBase.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Syntax/BuildTree.h"
 #include "clang/Tooling/Syntax/Nodes.h"
 #include "clang/Tooling/Syntax/TokenBufferTokenManager.h"
 #include "clang/Tooling/Syntax/Tree.h"
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
 #include <queue>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {

 // Related issue: https://github.com/clangd/clangd/issues/310
 llvm::Expected<std::vector<FoldingRange>> getFoldingRanges(ParsedAST &AST) {
   syntax::Arena A;
   syntax::TokenBufferTokenManager TM(AST.getTokens(), AST.getLangOpts(),
                                      AST.getSourceManager());
   const auto *SyntaxTree = syntax::buildSyntaxTree(A, TM, AST.getASTContext());
   return collectFoldingRanges(SyntaxTree, TM);
 }
 
-// FIXME(kirillbobyrev): Collect comments, PP conditional regions, includes and
-// other code regions (e.g. public/private/protected sections of classes,
-// control flow statement bodies).
+// FIXME( usaxena95): Collect PP conditional regions, includes and other code
+// regions (e.g. public/private/protected sections of classes, control flow
+// statement bodies).
 // Related issue: https://github.com/clangd/clangd/issues/310
 llvm::Expected<std::vector<FoldingRange>>
 getFoldingRanges(const std::string &Code) {
-  auto OrigStream = clang::pseudo::lex(Code, clang::pseudo::genericLangOpts());
+  auto OrigStream = pseudo::lex(Code, clang::pseudo::genericLangOpts());
 
-  auto DirectiveStructure = clang::pseudo::DirectiveTree::parse(OrigStream);
-  clang::pseudo::chooseConditionalBranches(DirectiveStructure, OrigStream);
+  auto DirectiveStructure = pseudo::DirectiveTree::parse(OrigStream);
+  pseudo::chooseConditionalBranches(DirectiveStructure, OrigStream);
 
   // FIXME: Provide ranges in the disabled-PP regions as well.
   auto Preprocessed = DirectiveStructure.stripDirectives(OrigStream);
 
   auto ParseableStream = cook(Preprocessed, clang::pseudo::genericLangOpts());
   pseudo::pairBrackets(ParseableStream);
 
   std::vector<FoldingRange> Result;
-  for (const auto &Tok : ParseableStream.tokens()) {
-    if (auto *Paired = Tok.pair()) {
-      // Process only token at the start of the range. Avoid ranges on a single
-      // line.
-      if (Tok.Line < Paired->Line) {
-        Position Start = offsetToPosition(
-            Code,
-            OrigStream.tokens()[Tok.OriginalIndex].text().data() - Code.data());
-        Position End = offsetToPosition(
-            Code, OrigStream.tokens()[Paired->OriginalIndex].text().data() -
-                      Code.data());
-        FoldingRange FR;
-        FR.startLine = Start.line;
-        FR.startCharacter = Start.character + 1;
-        FR.endLine = End.line;
-        FR.endCharacter = End.character;
-        Result.push_back(FR);
+  auto ToFoldingRange = [](Position Start, Position End,
+                           llvm::StringLiteral Kind) {
+    FR.startCharacter = Start.character;
+    FR.kind = Kind.str();
+    return FR;
+  };
+  auto OriginalToken = [&](const pseudo::Token &T) {
+    return OrigStream.tokens()[T.OriginalIndex];
+  };
+  auto StartOffset = [&](const pseudo::Token &T) {
+    return OriginalToken(T).text().data() - Code.data();
+  };
+  auto StartPosition = [&](const pseudo::Token &T) {
+    return offsetToPosition(Code, StartOffset(T));
+  };
+  auto EndPosition = [&](const pseudo::Token &T) {
+    return offsetToPosition(Code, StartOffset(T) + OriginalToken(T).Length);
+  };
+  auto Tokens = ParseableStream.tokens();
+  // Brackets.
+  for (const auto &Tok : Tokens) {
+    if (auto *Paired = Tok.pair()) {
+      // Process only token at the start of the range. Avoid ranges on a single
+      // line.
+      if (Tok.Line < Paired->Line) {
+        Position Start = offsetToPosition(Code, 1 + StartOffset(Tok));
+        Position End = offsetToPosition(Code, StartOffset(*Paired));
+        Result.push_back(ToFoldingRange(Start, End, FoldingRange::REGION_KIND));
       }
     }
   }
+  // Multi-line comments.
+  for (const auto *T = Tokens.begin(); T != Tokens.end();) {
+    if (T->Kind != tok::comment) {
+      T++;
+      continue;
+    }
+    Position Start = StartPosition(*T);
+    Position LastCommentEnd = EndPosition(*T);
+    while (T != Tokens.end() && T->Kind == tok::comment &&
+           StartPosition(*T).line <= LastCommentEnd.line + 1) {
+      LastCommentEnd = EndPosition(*T);
+      T++;
+    }
+    if (Start.line < LastCommentEnd.line)
+      Result.push_back(
+          ToFoldingRange(Start, LastCommentEnd, FoldingRange::COMMENT_KIND));
+  }
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp

const char *Tests[] = {
         \
         {[[  \
         public:
           Foo() {[[\
             int X = 1;
           ]]}   \
         ]]};
       )cpp",
+      R"cpp(
+        [[/* Multi 
+          * line
+          *  comment 
+          */]]
+      )cpp",
+      R"cpp(
+        [[// Comment
+        // 1]]
+        
+        [[// Comment
+        // 2]]
+        
+        // No folding for single line comment.
+
+        [[/* comment 3
+        */]]
+
+        [[/* comment 4
+        */]]
+      )cpp",
   };
   for (const char *Test : Tests) {
     auto T = Annotations(Test);
     EXPECT_THAT(
         gatherFoldingRanges(llvm::cantFail(getFoldingRanges(T.code().str()))),
         UnorderedElementsAreArray(T.ranges()))
         << Test;
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang