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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

just some random comments from my side.

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

+1. I think we need some general rules (mental model) to guide us how the folding works.

Some editors (e.g. VSCode) have their builtin folding-range, it is worth to investigate how the built folding-range interacts with one provided by the language server.

>From my experience, VSCode's built folding-range kind of works though this is not perfect, would be nice to have some concrete examples see the improvements, we may want to prioritize those during the implementation.



================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:261
+
+          if (B && ([["subexpression" != "multi-line" &&
+                    1 > 0]]))
----------------
this example seems vague, it is folded because 

1. it is cross-line
2. it is wrapped by `()`
or both?


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:284
+      )cpp",
+      // For.
+      R"cpp(
----------------
nit: for completeness, add a for-range case.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:326
+          int Y = 42;
+          bool B = 15;]]
+          if ([[B]]) {[[ ++X; ]]}
----------------
not sure about this case. 

if we decide to support this (btw, the test on L242 should be adjusted too), would be nice to support the following usages.

```
// forward decls
[[class A;
class B;
class C;]]

// using decls;
[[using ns::A;
using ns::B;
using ns::C;]]
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:556
+      R"cpp(
+        //[[ Some documentation.
+        // @param
----------------
this `[[` seems mismatched, if I read the code correctly.

btw, this example contains too many brackets, considering split into simpler ones.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:598
+
+        #include [["math.h"]]
+      )cpp",
----------------
another option: we could just fold the whole preamble region, it seems easier, and provides a visible to see preamble range :), the downside is that any comments/macros will be folded together.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:601
+      R"cpp(
+        #define true [[false]]
+
----------------
These macros are quite simple, I'm not sure this is useful to fold them. I'd just not fold it, but if the macro body is complicated (multi-lines), it is worth folding it.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:679
+        struct BarStruct {[[]]};
+        struct FwdDeclaration;
+      )cpp",
----------------
looks like we're missing case of inheritance, e.g. a class inherits multiple classes

```
class B : public A,
  private C {};
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83914