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