[PATCH] D105904: [clangd] Support `#pragma mark` in the outline

[Kadir Cetinkaya via Phabricator via cfe-commits]

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