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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks, mostly looks good couple of nits!



================
Comment at: clang-tools-extra/clangd/CollectMacros.cpp:42
+    if (isInsideMainFile(Loc, SM)) {
+      Position Start = sourceLocToPosition(SM, Loc);
+      Position End = {Start.line + 1, 0};
----------------
are we fine with these annotations spanning `#pragma [[mark XX]]` rather than the whole line or just `XX` ?


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:581
+      // Pragma owns all the children between P and NextPragma
+      auto It = std::partition(Cur->children.begin(), Cur->children.end(),
+                               [&P, &NextPragma](const auto &S) -> bool {
----------------
nit: `s/std::partition(Cur->children.begin(), Cur->children.end()/llvm::partition(Cur->children/`


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:641
+
+  std::vector<PragmaMarkSymbol> Pragmas;
+  for (const auto &P : PragmaMarks)
----------------
nit: Pragmas.reserve


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:645
+  Range EntireFile = {
+      {-1, -1},
+      {std::numeric_limits<int>::max(), std::numeric_limits<int>::max()}};
----------------
nit: `0,0` should suffice + makes sure the range is valid (in theory these are zero-based)


================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:651
+  Root.range = EntireFile;
+  mergePragmas(Root, PragmasRef);
+  return Root.children;
----------------
nit: `llvm::makeArrayRef(Pragmas)`


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:655
         llvm::iterator_range<const SourceLocation *> Locations) {
-      for (const auto &P : llvm::zip(Protocols, Locations)) {
         Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
this change does not look relevant, can you please revert?


================
Comment at: clang-tools-extra/clangd/ParsedAST.cpp:441
+  if (Preamble)
+    Marks = Preamble->Marks;
+  Clang->getPreprocessor().addPPCallbacks(
----------------
We are not patching marks for stale preambles, which I believe is fine but worth a FIXME. If you are interested please take a look at https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Preamble.cpp#L427, it should be fairly easy to handle movements of `#pragma mark`s in preamble section.


================
Comment at: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp:1055
+
+      #pragma $End[[mark - End
+]]
----------------
kadircet wrote:
> can you also add something like:
> 
> ```
> #pragma mark - Foo
> struct Foo {
>   #pragma mark - Bar
>   void bar() {
>      #pragma mark - NotTopDecl // I don't think we want this to be part of documentsymbol.
>   }
> };
> void bar() {}
> ```
can you add these cases as well to make sure details of the merge logic are working as expected?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105904/new/

https://reviews.llvm.org/D105904