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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
