dgoldman added a comment. In D105904#2877184 <https://reviews.llvm.org/D105904#2877184>, @kadircet wrote:
> 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. Yep, I can you send you some searches internally with how prevalent they are. > 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? Yep, although there are some TODOs there, we could allow letting leaf nodes to be contained in a group parent as well as potentially merging them in the `#pragma mark -` followed by `#pragma mark Foo` case. There's no good kind at the moment IMO, I don't really have a preference as long as it doesn't render awkwardly in VS Code. > (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.) I don't think this an Xcode limitation really - more of an LSP spec limitation. If we create a group inside of another symbol, we are effectively contained in it and cannot escape without violating the spec, right? To be clear, Xcode rules are a bit different - see the link above - the `#pragma mark -` just creates a divider which we can't mirror. > 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. This assumes (like this code today) that the nodes are sorted properly and the ranges of the parent are accurate. I was thinking of adding a proper sort in a post-processing phase but maybe we could do it + range fix up during insertion. Seems complicated to move everything over to insertion, but if you think that's reasonable, I can try that out. And that case generally works but we need to be sure to handle multiple pragmas next to each other. `#pragma mark - Foo\n#pragma mark - Bar` turns Foo into an empty group and Bar takes decls after. And then, currently, a `#pragma mark Third` would end the group as well. > - 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. I think it's a bit more complicated since the leaf nodes terminate the groups. As long as we do that it should be fine, assuming editors don't require it to be sorted. > 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? We could - I considered it, but with the existing behavior and potential subtle ordering issues I thought it would be simpler, but less efficient to do it after. I took a look at the Macro Expansion code and didn't understand it + potential edge cases well enough to add it there. 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
