dgoldman added inline comments.
================ Comment at: clang-tools-extra/clangd/FindSymbols.cpp:535 +/// by range. +std::vector<DocumentSymbol> mergePragmas(std::vector<DocumentSymbol> &Syms, + std::vector<PragmaMarkSymbol> &Pragmas, ---------------- dgoldman wrote: > 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. > > I've implemented your suggestion. I don't think it's simpler, but LMK, maybe it can be improved. 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
