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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
