kadircet 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:
> kadircet wrote:
> > 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.
> > >
> > >
> > ```
> > while(Pragmas) {
> > // We'll figure out where the Pragmas.front() should go.
> > Pragma P = Pragmas.front();
> > DocumentSymbol *Cur = Root;
> > while(Cur->contains(P)) {
> > auto *OldCur = Cur;
> > for(auto *C : Cur->children) {
> > // We assume at most 1 child can contain the pragma (as pragmas are on
> > a single line, and children have disjoint ranges)
> > if (C->contains(P)) {
> > Cur = C;
> > break;
> > }
> > }
> > // Cur is immediate parent of P
> > if (OldCur == Cur) {
> > // Just insert P into children if it is not a group and we are done.
> > // Otherwise we need to figure out when current pragma is terminated:
> > // if next pragma is not contained in Cur, or is contained in one of the
> > children, It is at the end of Cur, nest all the children that appear after
> > P under the symbol node for P.
> > // Otherwise nest all the children that appear after P but before next
> > pragma under the symbol node for P.
> > // Pop Pragmas and break
> > }
> > }
> > }
> > ```
> >
> > Does that make sense, i hope i am not missing something obvious?
> > Complexity-wise in the worst case we'll go all the way down to a leaf once
> > per pragma, since there will only be a handful of pragmas most of the time
> > it shouldn't be too bad.
> I've implemented your suggestion. I don't think it's simpler, but LMK, maybe
> it can be improved.
oops, i was looking into an older revision and missed mergepragmas2, i think it
looks quite similar to this one but we can probably get rid of the recursion as
well and simplify a couple more cases
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
