[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 20 13:39:47 PST 2018
sammccall added a comment.
Very nice! Mostly just a few style/structure nits.
================
Comment at: clangd/AST.cpp:69
+std::string printName(const NamedDecl &ND) {
+ const NamedDecl *NameSource = &ND;
+ std::string Name = llvm::to_string(NameSource->getDeclName());
----------------
just use ND?
================
Comment at: clangd/ClangdLSPServer.cpp:28
+/// Used by the clients that do not support the hierarchical view.
+static std::vector<SymbolInformation>
+flattenDocumentSymbols(llvm::ArrayRef<DocumentSymbol> Symbols,
----------------
move this nearer the call site?
================
Comment at: clangd/ClangdLSPServer.cpp:29
+static std::vector<SymbolInformation>
+flattenDocumentSymbols(llvm::ArrayRef<DocumentSymbol> Symbols,
+ const URIForFile &FileURI) {
----------------
or flattenSymbolHierarchy?
================
Comment at: clangd/ClangdLSPServer.cpp:31
+ const URIForFile &FileURI) {
+ // A helper struct to keep the state for recursive processing, computes
+ // results on construction.
----------------
auto-typed lambdas can't be recursive, but std::function shouldn't be too slow here? And clearer I think:
```
vector<SymbolInformation> Results;
std::function<void(const DocumentSymbol&, StringRef)> Process = [&](const DocumentSymbol &Sym, StringRef ParentName) {
...
};
for (const auto& TopLevel : Symbols)
Process(TopLevel);
return Results;
```
failing that, I found this pattern confusing - I'd suggest either making it a recursive plain function, or a standard functor object as if it were a lambda (with the recursive call being `operator()`)
================
Comment at: clangd/ClangdLSPServer.cpp:44
+ private:
+ void process(const DocumentSymbol &S, llvm::StringRef ParentName) {
+ SymbolInformation SI;
----------------
nit: optional<StringRef> for parent name seems slightly neater
================
Comment at: clangd/FindSymbols.cpp:190
+ index::SymbolInfo SymInfo = index::getSymbolInfo(&ND);
+ SymbolKind SK = indexSymbolKindToSymbolKind(SymInfo.Kind);
----------------
may want to add a FIXME here: per the tests, it's not classifying constructor/destructor/operator correctly (they're all "methods").
cons/dest is just indexSymbolKindToSymbolKind needing an update, but operator isn't represented in index::SymbolKind.
Maybe we should stop using index::SymbolKind entirely, it doesn't appear to be great.
================
Comment at: clangd/FindSymbols.cpp:208
+std::vector<DocumentSymbol> collectDocSymbols(ParsedAST &AST) {
+ struct CollectSymbols {
+ CollectSymbols(ParsedAST &AST, std::vector<DocumentSymbol> &Results)
----------------
this class seems maybe too long to live inside this function. (I wouldn't worry about it being visible to other stuff, the file is small and focused)
================
Comment at: clangd/FindSymbols.cpp:208
+std::vector<DocumentSymbol> collectDocSymbols(ParsedAST &AST) {
+ struct CollectSymbols {
+ CollectSymbols(ParsedAST &AST, std::vector<DocumentSymbol> &Results)
----------------
sammccall wrote:
> this class seems maybe too long to live inside this function. (I wouldn't worry about it being visible to other stuff, the file is small and focused)
(this looks more like a class than a struct?)
================
Comment at: clangd/FindSymbols.cpp:208
+std::vector<DocumentSymbol> collectDocSymbols(ParsedAST &AST) {
+ struct CollectSymbols {
+ CollectSymbols(ParsedAST &AST, std::vector<DocumentSymbol> &Results)
----------------
sammccall wrote:
> sammccall wrote:
> > this class seems maybe too long to live inside this function. (I wouldn't worry about it being visible to other stuff, the file is small and focused)
> (this looks more like a class than a struct?)
should this be a RecursiveASTVisitor?
I guess not, but please explain why (there's no documentation on the implementation strategy, and it's complicated)
================
Comment at: clangd/FindSymbols.cpp:211
+ : Ctx(AST.getASTContext()) {
+ for (auto &TopLevel : AST.getLocalTopLevelDecls())
+ traverseDecl(TopLevel, Results);
----------------
again, doing the work in the constructor is a bit odd - own the data and expose it?
================
Comment at: clangd/FindSymbols.cpp:253
- SymbolInformation SI;
- SI.name = Name;
- SI.kind = SK;
- SI.location = L;
- SI.containerName = Scope;
- Symbols.push_back(std::move(SI));
- return true;
- }
-};
-} // namespace
+ VisitKind shouldVisit(Decl* D) {
+ if (D->isImplicit())
----------------
we only consider visiting named decls, maybe reflect that here?
(not needed now but may allow future refinement)
================
Comment at: clangd/FindSymbols.cpp:264
+ // ignored.
+ if (auto *Info = Func->getTemplateSpecializationInfo()) {
+ if (!Info->isExplicitInstantiationOrSpecialization())
----------------
isn't this covered by D->isImplicit?
================
Comment at: clangd/FindSymbols.cpp:276
+ // children.
+ // - implicit instantiations, i.e. not written by the user.
+ // Do not visit at all, they are not present in the code.
----------------
isn't this covered by D->isImplicit() above?
================
Comment at: clangd/Protocol.h:43
InvalidParams = -32602,
+
InternalError = -32603,
----------------
why?
================
Comment at: clangd/Protocol.h:154
bool contains(Position Pos) const { return start <= Pos && Pos < end; }
+ bool containsSubrange(Range Rng) const {
+ return start <= Rng.start && Rng.end <= end;
----------------
just an overload of contains()?
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:48
+testing::Matcher<DocumentSymbol>
+Children(testing::Matcher<std::vector<DocumentSymbol>> ChildrenM) {
+ return Field(&DocumentSymbol::children, ChildrenM);
----------------
`ChildrenAre()` (with no args) and `NoChildren()` are the same matcher I think.
I'd suggest flattening Children() into ChildrenAre() and renaming it to Children() - less things for the reader to understand.
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+ SymNameRange(Main.range("decl"))))),
+ AllOf(WithName("f"), WithKind(SymbolKind::Method),
+ SymNameRange(Main.range("def")))));
----------------
this one is to be fixed, right?
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+ ChildrenAre(AllOf(WithName("x"), WithKind(SymbolKind::Field)))),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+ ChildrenAre(WithName("y"))),
----------------
hmm, this seems pretty confusing - I think `Tmpl<float>` would be a clearer name for a specialization, even if we just have `Tmpl` for the primary template.
Partial specializations are confusing, though :-/
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+ ChildrenAre(WithName("y"))),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren())));
----------------
why the change in policy with this patch? (one of these previously was deliberately not indexed, now is)
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:524
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren())));
}
----------------
these assertions are confusing as it's not clear which expected belongs with which piece of actual code. Obviously they all need to have the same name though.
Best workaround I can think of is putting "int a", "int b" etc between the confusing decls, and including them in the expected top-level symbols (since you assert in order)
================
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
addFile(FilePath, R"(
enum {
Red
----------------
hmm, our handling of anonymous enums seems different than anonymous namespaces - why?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
More information about the cfe-commits
mailing list