[PATCH] D131154: FoldingRanges: Handle LineFoldingsOnly clients.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, i think we should change the behaviour to not fold the last line in any case.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:943
     Callback<std::vector<FoldingRange>> Reply) {
-  Server->foldingRanges(Params.textDocument.uri.file(), std::move(Reply));
+  Server->foldingRanges(Params.textDocument.uri.file(), LineFoldingsOnly,
+                        std::move(Reply));
----------------
normally this is an LSP specific behaviour, hence we should do the conversion of clangd internal representation here (in the LSP layer). but it feels like the line-only foldings are actually the sensible default for most of the clients so it makes sense to do it inside the clangdserver (also the information needed might not be present at this point.

but still rather than passing it with every request, let's make it an option of clangdserver and don't pass it here.


================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:263
+  // Whether the client supports folding only complete lines.
+  bool LineFoldingsOnly = false;
   std::mutex BackgroundIndexProgressMutex;
----------------
`LineFoldingOnly` (as spelled in LSP)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:202
     FR.endLine = End.line;
-    FR.endCharacter = End.character;
+    if (LineFoldingsOnly)
+      FR.startCharacter = FR.endCharacter = 0;
----------------
can we do this as part of the adjust logic at the bottom?

moreover, i don't think we actually need to care about these. spec is clear about these values being ignored by line-folding-only clients anyway.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:225
+                                       Position End) {
+    // For LineFoldingsOnly clients, do not fold the last line if it
+    // contains tokens after `End`.
----------------
i don't think the extra check for "if there are more tokens" is really useful here, in a scenario like:
```
if (foo) {
  ...
}
```
do we actually want to fold away the closing `}` as well ? (i don't think we do)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:227
+    // contains tokens after `End`.
+    if (LineFoldingsOnly &&
+        EndToken.OriginalIndex + 1 < OrigStream.tokens().size() &&
----------------
nit: early bail out

`if(!LineFoldingsOnly) return;`


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:241
       if (Tok.Line < Paired->Line) {
-        Position Start = offsetToPosition(Code, 1 + StartOffset(Tok));
-        Position End = StartPosition(*Paired);
-        Result.push_back(ToFoldingRange(Start, End, FoldingRange::REGION_KIND));
+        const Position Start = offsetToPosition(Code, 1 + StartOffset(Tok));
+        const Position End =
----------------
similarly no consts here


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:254
     }
-    Position Start = StartPosition(*T);
-    Position LastCommentEnd = EndPosition(*T);
+    const Position Start = StartPosition(*T);
+    const pseudo::Token *LastComment = T;
----------------
let's not introduce the consts here (i know the clang-tidy check is complaining but the check itself should be disabled for LLVM instead, that's not what the codebase adheres to)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:372
+      R"cpp(
+        void func(int a) {[[
+          if(a == 1) {[[
----------------
so IIUC outer-most folding here will imply:
```
void func(int a) {
...
```

which doesn't feel right, i think it should look like:
```
void func(int a) {
...
}
```

which implies not having the last line included in the range.

similarly for the folding near `else` i think we would get:
```
void func(int a) {
  if(a == 1) {
   a++;
  } else {
   ...
}
```

which again doesn't look right (as user can still see the opening brace, but not the closing brace), we shouldn't be folding the closing `}` in any case.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:391
+        /* No folding for this comment.
+        */ int b_token=0;
+      )cpp",
----------------
it'd be nice to have a test case like:
```
template <[[typename foo, class bar]]> struct baz {[[]]};
```

which isn't useful for line-only folding clients, but something we should be able to support going forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131154