Organize symbol hierarchy in LSP documentSymbol handler #4914
Conversation
d4b16b7 to
33d308f
Compare
| } | ||
|
|
||
| private: | ||
| std::vector<clang::clangd::DocumentSymbol> top_level_symbols_; |
There was a problem hiding this comment.
Should this be a SmallVector too? Slightly curious to see a std::vector next to a SmallVector (would usually expect one or the other to be used consistently - is there some difference you're relying on/this patch benefits from in some way?)
There was a problem hiding this comment.
The output of HandleDocumentSymbol needs to be a std::vector since thats the llvm::json::Value compatible type, using SmallVector here would then require a copy into a regular vector so I use std::vector for the output.
| } | ||
|
|
||
| auto Collect() -> std::vector<clang::clangd::DocumentSymbol> { | ||
| // Shouldn't happen in a valid tree but may as well handle gracefully. |
There was a problem hiding this comment.
I /tend/ to be an assert-first, loosen if necessary later. In case there are broken things, the sooner we know about it the better, perhaps?
There was a problem hiding this comment.
Fair, I've been reluctant to place too many CARBON_CHECKS within the language server though, as it often results in crash loops. But agree probably should move whatever error handling outside of the helper class so that it can perform more appropriate warnings, e.g. emitting a diagnostic.
| class SymbolStore { | ||
| public: | ||
| // Adds a symbol with no children. | ||
| void AddSymbol(clang::clangd::DocumentSymbol symbol) { |
There was a problem hiding this comment.
It doesn't look like the codebase is entirely consistent here, but it does say to always use late bound return types, and there are many examples of "-> void" in the codebase - so maybe we should go with that here?
There was a problem hiding this comment.
Fixed, initially I assumed void was the exception to that rule since clang-tidy doesn't warn about it, but later saw that auto Foo() -> void was still used.
| @@ -35,6 +37,44 @@ static auto GetIdentifierName(const Parse::TreeAndSubtrees& tree_and_subtrees, | |||
| return std::nullopt; | |||
| } | |||
|
|
|||
| class SymbolStore { | |||
There was a problem hiding this comment.
Quick overview class comment might be handy
Head branch was pushed to by a user without write access
This PR updates the HandleDocumentSymbol function to build a tree of symbols, rather than a flat list. Symbols found while traversing another symbols body are added as a child.
Adds a simple test showing functions within nested class.