dgoldman added inline comments.
================
Comment at: clang-tools-extra/clangd/CollectMacros.h:106
+struct PragmaMark {
+ SourceLocation Loc;
+ std::string Text;
----------------
sammccall wrote:
> Line number is enough, right?
> Column is not interesting, and pragma marks expanded from macros are not
> possible (and wouldn't be in the main file for our purposes)
No, I think the column is useful, ideally we would point directly to the text
that the user has although currently we point to the mark symbol.
================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:535
+/// by range.
+std::vector<DocumentSymbol> mergePragmas(std::vector<DocumentSymbol> &Syms,
+ std::vector<PragmaMarkSymbol>
&Pragmas,
----------------
sammccall wrote:
> FWIW the flow control/how we make progress seem hard to follow here to me.
>
> In particular I think I'm struggling with the statefulness of "is there an
> open mark group".
>
> Possible simplifications:
> - define a dummy root symbol, which seems clearer than the vector<symbols> +
> range
> - avoid reverse-sorting the list of pragma symbols, and just consume from
> the front of an ArrayRef instead
> - make the outer loop over pragmas, rather than symbols. It would first
> check if the pragma belongs directly here or not, and if so, loop over
> symbols to work out which should become children. This seems very likely to
> be efficient enough in practice (few pragmas, or most children are grouped
> into pragmas)
> define a dummy root symbol, which seems clearer than the vector<symbols> +
> range
I guess? Then we'd take in a `DocumentSymbol & and a ArrayRef<PragmaMarkSymbol>
& (or just by value and then return it as well). The rest would be the same
though
> In particular I think I'm struggling with the statefulness of "is there an
> open mark group".
We need to track the current open group if there is one in order to move
children to it.
> make the outer loop over pragmas, rather than symbols. It would first check
> if the pragma belongs directly here or not, and if so, loop over symbols to
> work out which should become children. This seems very likely to be efficient
> enough in practice (few pragmas, or most children are grouped into pragmas)
The important thing here is knowing where the pragma mark ends - if it doesn't,
it actually gets all of the children. So we'd have to peak at the next pragma
mark, add all symbols before it to us as children, and then potentially recurse
to nest it inside of a symbol. I'll try it out and see if it's simpler.
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
