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