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

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

In D105904#2879445 <https://reviews.llvm.org/D105904#2879445>, @kadircet wrote:

> In D105904#2877281 <https://reviews.llvm.org/D105904#2877281>, @dgoldman wrote:
>
>> Yep, I can you send you some searches internally with how prevalent they are.
>
> Thanks!
>
>> 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.
>
> Sorry I don't follow what you mean here. I was talking about inability to have something like:
>
>   #pragma mark -Foo
>     #pragma mark -Bar
>     #pragma mark SPECIAL_BAR_END
>   #pragma mark SPECIAL_FOO_END
>
> By making sure groups are terminated **explicitly** we gain the ability to nest them, whereas otherwise `-Bar` above will always terminate the previous group implicitly.

Ah I see what you mean. Xcode has implicit termination - the #pragma mark - Foo just makes a horizontal line and labels Foo after, so a subsequent #pragma mark - Bar ends up creating a new line, ending the previous. There's too much relying on this behavior for us to change it. My point was we can't avoid an implicit end here of the Foo mark due to how we nest things, so either way we'd have some form of implicit termination:

  @interface Foo
  #pragma mark - Foo
  @end



In D105904#2879750 <https://reviews.llvm.org/D105904#2879750>, @sammccall wrote:

> However those difficulties don't apply here - I believe you can take the current final representation (`vector<DocumentSymbol>`) + the list of marks and produce a markified `vector<DocumentSymbol>` via a fairly natural tree walk. And this does a *really* good job of isolating this feature - we literally run the rest of the algorithm without it.
> (This is slightly different to what's done in the patch, I would actually take it out of the `DocumentOutline` class entirely).
> I don't think this hurts efficiency much (I think/hope this ends up being a move rather than a copy for most nodes). And I think it actually makes the markifying code easier to reason about in this case, as it just deals with line numbers/positions, without having the distraction of SourceLocation.

That seems reasonable, it seems quite similar to this, we can probably simplify it if we remove the requirement that the children are in order. Note that my current approach doesn't really add much overhead if the absence of the TextMarks, but we should be able to special case that there as well.



================
Comment at: clang-tools-extra/clangd/TextMarks.h:22
+/// Represents a programmer specified mark/note, typically used to easily locate
+/// or differentiate code. e.g. a `pragma mark`, a `TODO`.
+///
----------------
sammccall wrote:
> This seems like premature abstraction - you don't handle TODOs here, nor are they appropriate for the single use case we have. Moreover they don't satisfy the definition here: `int foo; // TODO: turn into a float` the TODO doesn't occupy the whole line.
> 
> If we need it, the appropriate generalization seems just as likely to be "pragmas" or "directives" as "textual marks".
> 
> Similarly, it's not clear that the logic around interpreting the strings e.g. as groups needs to be shared across features, nor is it terribly complicated.
> So the interface here seems like it could be much simpler:
> ```
> struct PragmaMark {
>   unsigned Line;
>   string Text;
> }
> unique_ptr<PPCallbacks> collectPragmaMarksCallback(const SourceManager&, vector<PragmaMark> &Out);
> ```
> 
> I'd suggest putting this in CollectMacros.h (which would be misnamed but not terribly, or we could always rename) rather than adding a new header.
We don't handle them at the moment, but Xcode does (for both Swift + ObjC): https://medium.com/@cboynton/todo-make-your-notes-on-xcode-stand-out-5f5124ec064c. You're right that they don't necessarily have to occupy the whole line though, it's possible to have multiple per line although I'm not sure how often that is used in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105904