[PATCH] D105904: [clangd] Support `#pragma mark` in the outline

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

So first of all, it is not clear to me if this is a useful enough improvement to justify all the new code, but looking at your investment I'll assume this is really common in ObjC land and gonna be useful for developers working with ObjC. That's enough of a justification for me, but I'd be more than happy to see some real data if you have any.

That being said, reading the code it seems like the intent is enabling `#pragma mark` directives the introduce symbols that are either leaves or containers:

- Leaf case is obvious and straightforward, it is just like a normal declaration happening at that particular location. It is unclear which kind should be used. It seems you've went with `File` but I don't think it is suitable in this case. I would suggest using `Null`.
- Container case is more sophisticated. One thing is for sure, they are nested under the same symbol a declaration occuring at that point would be. They also seem to contain all the symbols that would nest under current subtree and spelled after the mark and spelled before any other marks. Hence there is no way to explicitly terminate the scope of a mark. They are implicitly terminated by hitting the end of the container containing the mark or hitting a new mark directive. This also implies one cannot have multiple levels of nesting just by pragma directives, but it can be achieved if there are usual containers with more mark directives (see my comment around tests for an example).

Can you verify that I understood the intent correctly and make it more explicit in the comments as well?

(Terminating containers implicitly sounds like an unfortunate limitation to me. I'd rather only enable termination of them explicitly) but I suppose there's a lot of code that's already existing out in the world with the assumption of XCode's implicit termination rules, so this is probably not gonna be helpful in reality. Hence we can't do much.)

Before diving into the details of the implementation it looks like we are doing some post-processing to incorporate mark pragmas into the tree, why can't we just insert them while building the tree as we do with macros? for example:

- when inserting a symbol we look for a possible mark-pragma container (similar to macro case), a mark pragma is only eligible as a container if it is contained within the range of the parent && spelled before current declaration at hand && is a group.
- as for insertion of leaves, after traversal of all the children we can just add any mark pragmas that occured within the current symbol's range as a child. we can also drop any mark pragmas occuring inside symbols that doesn't require child traversal this way.

This approach relies on the fact that `range` calculated by `declToSym` will be correct enough. I think it should be good in most cases, but might require some adjustments around macros, as usual (can't think of a problematic example right now though).
This brings out the question about ordering of `children` in `DocumentSymbol`, lsp doesn't say anything about it today, but during rendering we can just order these based on the `selectionRange` (probably during `build`) and hope that clients will preserve it.

I believe implementing it as part of the traversal rather than as post processing both makes code easier to reason about and more efficient. So WDYT about going in that direction?



================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:1053
+      }]]
+      @]]end  // FIXME: Why doesn't this include the 'end'?
+
----------------
probably a little mix of token vs char ranges.


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:1055
+
+      #pragma $End[[mark - End
+]]
----------------
can you also add something like:

```
#pragma mark - Foo
struct Foo {
  #pragma mark - Bar
  void bar() {
     #pragma mark - NotTopDecl // I don't think we want this to be part of documentsymbol.
  }
};
void bar() {}
```


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:1122
+                          AllOf(WithName("someFunctionAbove")),
+                          // FIXME: This should be nested under MYObject below.
+                          AllOf(WithName("someHelperFunction")),
----------------
yikes, this looks bad, but also a little bit confusing to be added with this change. Can you move it to a separate one? (or just create a bug report in github/clangd/clangd).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105904