[PATCH] D130081: Add foldings for multi-line comment.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Protocol.h:1782
   unsigned endCharacter;
-  std::string kind;
+  FoldingRangeKind kind;
 };
----------------
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. 


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:232
+    while (T != Tokens.end() && T->Kind == tok::comment &&
+           LastComment->Line == T->Line + 1) {
+      LastComment = T;
----------------
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).


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:351
+        int c;
+        [[// A comment 
+        // expanding more than
----------------
usaxena95 wrote:
> hokein wrote:
> > 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
> > ```
> 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.
> 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 {};
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130081