[PATCH] D131154: FoldingRanges: Handle LineFoldingsOnly clients.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:202
     FR.endLine = End.line;
+    FR.startCharacter = Start.character;
     FR.endCharacter = End.character;
----------------
can you put it back to its previous location (i.e. right after startline)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:251
+      // Always show last line of a block comment.
+      if (StartPosition(*LastComment).line != EndPosition(*LastComment).line) {
+        End.line--;
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:391
+        /* No folding for this comment.
+        */ int b_token=0;
+      )cpp",
----------------
usaxena95 wrote:
> kadircet wrote:
> > 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.
> 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.
> 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131154