[PATCH] D83914: [clangd] Plan features for FoldingRanges

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

A few different types of feedback mixed up here :-)

Some nitpicks about behavior: these are probably worth discussing.

Places where the coverage is incomplete: obviously until we get closer to implementation, how complete this is is up to you.

Looking at examples mostly reinforced to me that foldability should have some threshold for what's inside as well as rules about grammatically what's foldable. I just can't see editors doing a good job of discarding uninteresting ranges - they're going to consider that our job.
If "only things spanning newlines" is too strict, we could consider maybe other complexity heuristics.

At least for the AST-based folds, I'd like some formalism like:

- we build a tree of potentially-foldable regions based on the AST
- starting at the bottom, we decide whether each potentially-foldable region is actually folded
  - any region containing a non-folded newline is folded
  - any bracket-delimited region containing a non-folded comma or semicolon token is folded

Regarding implementation and RAV: we talked about the possibility of using syntax trees, is that still on the table?



================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:237
 
+TEST(FoldingRanges, ControlFlow) {
+  const char *Tests[] = {
----------------
nit: the subexpression tests are mixed in with the if cases


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:246
+
+          if ([[B && I > 42 || ~([[++I]])]]) {[[
+            ++I;
----------------
confused about [[++I]] - why is this foldable but not I > 42 etc - is this because of the containing parens?

FWIW, Personally, I'd want the whole if condition to be  foldable (maybe only if it spans lines), and only paren subexpressions, arg lists etc that span lines to be foldable. It's hard to imagine a UX that could expose folding small parens in a way that's not annoying. (e.g. shows lots of extra UI elements, or prevents folding the outer expression)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:277
+        int main() {[[
+          for ([[[[int I = 0]];[[I < 42]];[[++I]]]]) {[[
+            --I;
----------------
this is a really nice model, but I worry that having multiple folding ranges starting at the same point within a line is going to be a UX nightmare.
I'd suggest picking one.

(Note that other control flow statements can look like for loops too these days, with initializer statements)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:329
+      )cpp",
+      // Argument lists.
+      R"cpp(
----------------
missing lambdas, blocks, objc methods.
cover bodies in the same tests?
capture lists of lambdas?


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:342
+      )cpp",
+      // Namespace.
+      R"cpp(
----------------
linkage specs probably belong here too


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:414
+        template <[[unsigned U]]>
+        void foo<U, 50>([[char t, U u]]) {}
+
----------------
why is the template argument list not foldable here? (it is for the class example above)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:552
+        #include "external/Logger.h"
+        #include "external/Vector.h"]]
+        #include [["project/DataStructures/Trie.h"
----------------
I think clang-format messed some of these up for you


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:578
+        //[[ contents of the header]]
+        #endif]] /*[[ LIBRARY_FILENAME_H ]]*/
+
----------------
I think consistent with how you handle braces and  comments, the range should end just before the #endif


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:580
+
+        #if [[__has_include(<optional>)
+        #  include <optional>
----------------
I don't think you want the condition as part of the same of the same fold as the content, it might be important

What about
```
#if [[some_condition]][[
body
]]#elif [[some_condition]][[
body
]]#else[[
body
]]#endif
```

Rendering (partially folded) as:
#if some_condition ... #elif some_condition ... #else ... #endif


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:583
+        #  define have_optional 1
+        #]]elif __has_include(<experimental/optional>) [[]]
+        #  include <experimental/optional>
----------------
here I'd expect folding before # so you can see it's a directive when folded


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:605
+      R"cpp(
+        #error [["Not a standard compliant compiler"]]
+
----------------
I think YAGNI here - not sure how this could help, and it's an obscure feature to start with


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:613
+      R"cpp(
+        #line [[777 FNAME]]
+
----------------
Similarly.
*Maybe* generically folding directives that span multiple lines with \. But I think they're rare enough nobody cares.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:628
+
+TEST(FoldingRanges, Classes) {
+  const char *Tests[] = {
----------------
would be lovely to cover objc @interface/@implementation too...


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:656
+      )cpp",
+      // Accessor secions.
+      R"cpp(
----------------
You're missing the case where there's stuff in the implicit initial section, and then another accessor section.

It's an interesting one because if you allow the implicit section to be folded, and also the whole class to be folded, then you have two folds triggered at the same location. (Currently there's only one [[[[ in this file, which i've flagged)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:718
+      )cpp",
+      // Initializer lists.
+      R"cpp(
----------------
nit: these are "member initializer lists", not to be confused with "initializer lists" which are the braced things (and should also be covered, I guess!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83914