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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits