kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/CollectMacros.cpp:13
+namespace {
+class CollectPragmaMarks : public clang::PPCallbacks {
----------------
can you nest this inside `clang::clangd` and drop the qualifiers ?
================
Comment at: clang-tools-extra/clangd/CollectMacros.cpp:20
+ void PragmaMark(clang::SourceLocation Loc, clang::StringRef Trivia) override
{
+ if (clang::clangd::isInsideMainFile(Loc, SM)) {
+ clang::StringRef Name = Trivia.trim();
----------------
nit: drop braces
================
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)
okay then let's store a `Range` here instead, and get rid of the dependency on
a SourceManager for rendering it maybe?
================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:682
+ // here since editors won't properly render the symbol otherwise.
+ StringRef MaybeGroupName = Name;
+ if (MaybeGroupName.consume_front("-") &&
----------------
I think this reads easier:
```
bool IsGroup = Name.consume_front("-");
Name = Name.ltrim();
if (Name.empty())
Name = IsGroup ? "unnamed group" : ...;
```
================
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.
>
>
```
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.
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
