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