Add foldings for multi-line comment.

[hokein]

clang-tools-extra/clangd/SemanticSelection.cpp

176

nit: update the fixme, comment is supported now.

231

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

235

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

1782 ↗

(On Diff #445893)

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

231

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

1782 ↗

(On Diff #445893)

+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

231

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

[usaxena95]

Remove unintended changes.

[hokein]

clang-tools-extra/clangd/SemanticSelection.cpp

178

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

216–218

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

231

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/SemanticSelection.cpp

 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).
 // 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, std::string Kind) {
+    FR.startCharacter = Start.character;
+    FR.kind = Kind;
+    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 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, "region"));
+      }
+    }
+  }
+  // Multi-line comments.
+  for (const auto *T = Tokens.begin(); T != Tokens.end();) {
+    if (T->Kind != tok::comment) {
+      T++;
+      continue;
+    Position Start = offsetToPosition(Code, StartOffset(*T));
+    Position End;
+    while (T->Kind == tok::comment && T != Tokens.end()) {
+      End = offsetToPosition(Code, StartOffset(*T) + OriginalToken(*T).Length);
+      T++;
     }
+    if (Start.line < End.line)
+      Result.push_back(ToFoldingRange(Start, End, "comment"));
   }
   return Result;
 }
 
 } // namespace clangd
 } // namespace clang

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

const char *Tests[] = {
         \
         {[[  \
         public:
           Foo() {[[\
             int X = 1;
           ]]}   \
         ]]};
       )cpp",
+      R"cpp(
+        int a;
+        [[/* Multi 
+    line
+            comment */]]
+        
+        int b;
+        [[/* Multi 
+          * line
+          *  comment 
+          */]]
+        
+        int c;
+        [[// A comment 
+        // expanding more than
+
+        // one
+
+        // line.]]
+      )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