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