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