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

[David Goldman via Phabricator via cfe-commits]

dgoldman added inline comments.


================
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:
> 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.
Thought of another use case - for code folding it would be nice to let the #pragma marks fold. Not sure if ya'll are planning to ship clangd's code folding though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105904