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

[Sam McCall via Phabricator via cfe-commits]

sammccall 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`.
+///
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105904