Page MenuHomePhabricator

FoldingRanges: Handle LineFoldingsOnly clients.
ClosedPublic

Authored by usaxena95 on Aug 4 2022, 3:47 AM.

Details

Summary

Do not fold the endline which contains tokens after the end of range.

Diff Detail

Event Timeline

usaxena95 created this revision.Aug 4 2022, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 3:47 AM
usaxena95 requested review of this revision.Aug 4 2022, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 3:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
usaxena95 updated this revision to Diff 449928.Aug 4 2022, 4:18 AM

Do same for multi-line comments.

usaxena95 updated this revision to Diff 449931.Aug 4 2022, 4:35 AM

More tests.

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

clang-tools-extra/clangd/ClangdLSPServer.cpp
949

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.

clang-tools-extra/clangd/ClangdLSPServer.h
263 ↗(On Diff #449931)

LineFoldingOnly (as spelled in LSP)

clang-tools-extra/clangd/SemanticSelection.cpp
203

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.

224

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)

226

nit: early bail out

if(!LineFoldingsOnly) return;

231–233

similarly no consts here

248–252

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)

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

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.

405

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.

hokein added a comment.Aug 8 2022, 5:49 AM

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

+1, I think we should always do it for all bracket cases, but we probably don't want it for other cases (comment), right?

clang-tools-extra/clangd/Protocol.cpp
395

nit: remove the {} for the single-statement body.

clang-tools-extra/clangd/SemanticSelection.cpp
224

Yeah, it also introduces some inconsistencies (depending on whether there are trailing comments after the closed brackets).

We probably want the folding to work the same way for the following cases.

namespace foo {
 ...
} // namespace foo
namespace foo {
  ...
}
kadircet added inline comments.Aug 8 2022, 7:37 AM
clang-tools-extra/clangd/SemanticSelection.cpp
224

right. i think for comments we've got a bunch of different cases. for the block comments, i think we want to exclude the last line all the time again, e.g:

/*[[ foo
     bar
]]*/

i got 2 reasons:
1- this is somewhat the same as being able to see opening and closing brace.
2- when there's any token following the comment-block terminator, we actually mustn't hide them.

for back-to-back single-line comments, we should always fold the last line as well, eg:

//[[ foo
// bar]]

so i think the model here is always include the first line and don't include the last line if there are tokens after the folding range on the same line, as you've already proposed here, but we need to make sure that folding range doesn't include the terminators (like } and */). That way we'll ensure a line with a terminator is always visible.

nridge added a subscriber: nridge.Aug 13 2022, 3:12 PM
usaxena95 updated this revision to Diff 455920.Aug 26 2022, 8:32 AM
usaxena95 marked 11 inline comments as done.

Addressed comments.

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

I don't understand. We do not return any single-line ranges.
I think lines are never long enough to have helpful foldings (atleast in formatted code). It would only clutter IMO.

kadircet added inline comments.Aug 29 2022, 4:24 AM
clang-tools-extra/clangd/SemanticSelection.cpp
203

can you put it back to its previous location (i.e. right after startline)

261

i think this will end up trimming the foldings for cases like:

// foo\
bar\
baz

can we check the comment introducer instead? i.e. // vs /*. also let's amend the startposition to be Startoffset +2 similar to what we do with braces, to make sure starting sentinel is not included in the folding range.

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

I don't understand. We do not return any single-line ranges.

I think that's just a convenient solution we went with today because it's convenient and easier. i don't think that's the final UX we want to have.

I think lines are never long enough to have helpful foldings (atleast in formatted code). It would only clutter IMO.

Even if that was the case template arguments can get really long, especially in the presence of SFINAE, so there's definitely value in folding them.

Also just to be clear, i wasn't suggesting to have the implementation for them. I was just saying let's leave the testcases here, in disabled state, to remind us to have support for them in the future (and properly handle even for single line case).

usaxena95 updated this revision to Diff 456321.Aug 29 2022, 5:36 AM
usaxena95 marked 3 inline comments as done.

Address comments.

kadircet added inline comments.Aug 29 2022, 6:19 AM
clang-tools-extra/clangd/SemanticSelection.cpp
260

this is actually not true, we might have // foo */, so we need to check if it has /* in the beginning again.

also can we have it in an explicit variable instead bool IsBlockComment

265

so it actually gets pretty annoying with mixture of block and line comments back to back, but also with multiple block comments back to back, e.g:

/* foo
  * bar*/

vs

/* foo */
/* bar */

vs

// foo
/* bar */

which is rare, so i don't think it's worth handling. but at least to make it consistent, rather than checking for the start/end lines of the lastcomment, we should actually be looking at the whole range and whether we've started the range with a blockcomment. (sorry for noticing it too late)

usaxena95 updated this revision to Diff 456335.Aug 29 2022, 7:21 AM
usaxena95 marked 3 inline comments as done.

Addressed comments.

kadircet accepted this revision.Aug 29 2022, 8:10 AM

thanks, lgtm!

This revision is now accepted and ready to land.Aug 29 2022, 8:10 AM
This revision was automatically updated to reflect the committed changes.