sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/CollectMacros.cpp:23
+ bool IsGroup = false;
+ // "-\s+<group name>" or "<name>" after an initial trim. The former is
+ // considered a group, the latter just a mark. We need to include a name
----------------
Interpretation of the marks belongs with the rest of the DocumentSymbol code
too, I think - until we have some evidence that this is structure we want to
share.
================
Comment at: clang-tools-extra/clangd/CollectMacros.cpp:67
+
+DocumentSymbol PragmaMark::toSymbol(const SourceManager &SM) const {
+ DocumentSymbol Sym;
----------------
This code belongs with the rest of the DocumentSymbol code
================
Comment at: clang-tools-extra/clangd/CollectMacros.h:106
+struct PragmaMark {
+ SourceLocation Loc;
+ std::string Text;
----------------
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)
================
Comment at: clang-tools-extra/clangd/CollectMacros.h:115
+std::unique_ptr<PPCallbacks>
+collectPragmaMarksCallback(const SourceManager &, std::vector<PragmaMark>
&Out);
+
----------------
this should be tested in CollectMacrosTests.cpp or ParsedASTTests.cpp
(It's fine to test via TestTU though, and this will let you test both the
preamble and non-preamble case)
================
Comment at: clang-tools-extra/clangd/FindSymbols.cpp:535
+/// by range.
+std::vector<DocumentSymbol> mergePragmas(std::vector<DocumentSymbol> &Syms,
+ std::vector<PragmaMarkSymbol>
&Pragmas,
----------------
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)
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
