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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks. AFAIK this is now waiting on you David with Kadir to review impl.

In D105904#2881329 <https://reviews.llvm.org/D105904#2881329>, @dgoldman wrote:

> I think the difficulty is when you "figure out the mark container for a symbol" since we implicitly scope the mark containers based on their location, it's possible the closest #pragma isn't the right container. I'm not sure of a simple way to do that without sorting, LMK if you have some ideas.

I tried to work through this and yes I think you need the containers to be sorted. (If only because otherwise you have to reorder children within a container to group them, which is bizarre). If we do this with postprocessing, then where we fix-up the ranges is a good time to sort too. And mark insertion can preserve range nesting/sorting.
If the nesting is "incorrect" (siblings are not disjoint as in the function-in- at implementation example you gave) there doesn't seem to always be a great answer. For now I think it's OK to do something "wrong" in that case, as long as it's not infloop/crash.



================
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`.
+///
----------------
dgoldman wrote:
> sammccall wrote:
> > dgoldman wrote:
> > > 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.
> > This seems like a very different case, and I'm not convinced we should support it.
> > 
> > That `#pragma mark`s are used for grouping seems like the strongest argument for including them in the outline.  TODOs are not used for grouping.
> > 
> > The majority of `#pragma mark`s are written with XCode's conventions in mind, the majority of TODO comments are not. So there's a real question of whether authors want this. (And whether it should include all comments, or other kinds of structured comments...)
> > 
> > I'd suggest keeping the scope small and concrete. If you'd like to add abstractions because more cases are imminent, we probably need to get consensus on (some of) these cases first.
> > 
> > 
> Technically Xcode also supports `// MARK` comments as well, but almost all users internally use `#pragma mark`. I do think the TODO/FIXME in the outline could be useful (Xcode does it...), but if you're against it I'll remove this.
> 
> 
I think `// MARK` is so rarely used to be worth ignoring for now.

I think including TODO/FIXME is *probably* not great (unlike xcode, we can't tailor the UI to it and it doesn't seem to fit LSP). 

We can always discuss and work out generalize later though, mostly I'm against adding the abstraction now just in case we have the discussion later.


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:1122
+                          AllOf(WithName("someFunctionAbove")),
+                          // FIXME: This should be nested under MYObject below.
+                          AllOf(WithName("someHelperFunction")),
----------------
dgoldman wrote:
> kadircet wrote:
> > 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).
> I'm not sure this is worth disregarding just yet, if we're not able to fix this at the clang level we might want to consider a clangd-style fix here (which actually would behave similar to what we have right now with the text marks needing to be merged in).
Let's not disregard it, but we definitely need to think about it in a comprehensive way that isn't specific to this patch.

e.g. all selection operations (hover, go-to-definition, etc) fail inside such a function, because it violates the almost-rule that the AST corresponds to the lexical structure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105904