ioeric added inline comments.
================ Comment at: clangd/index/SymbolCollector.cpp:69 +// qualifier. Inline namespaces and unscoped enums are skipped. +llvm::Expected<std::string> getScope(const NamedDecl *ND) { + llvm::SmallVector<llvm::StringRef, 4> Contexts; ---------------- hokein wrote: > There is a `SuppressUnwrittenScope` option in `PrintingPolicy`, I think we > can probably use `printQualifiedName` with our customized policy (setting > `SuppressUnwrittenScope` to true) here. This is perfect! Thank you! ================ Comment at: clangd/index/SymbolCollector.cpp:73 + Context = Context->getParent()) { + if (llvm::isa<TranslationUnitDecl>(Context) || + llvm::isa<LinkageSpecDecl>(Context)) ---------------- sammccall wrote: > I'm not sure this is always correct: at least clang accepts this code: > > namespace X { extern "C++" { int y; }} > > and you'll emit "y" instead of "X::y". > > I think the check you want is > > if (Context->isTransparentContext() || Context->isInlineNamespace()) > continue; > > isTransparentContext will handle the Namespace and Enum cases as you do > below, including the enum/enum class distinction. > > (The code you have below is otherwise correct, I think - but a reader needs > to think about more separate cases in order to see that) In `namespace X { extern "C++" { int y; }}`, we would still want `y` instead of `X::y` since C-style symbol doesn't have scope. `printQualifiedName` also does the same thing printing `y`; I've added a test case for `extern C`. I also realized we've been dropping C symbols in `shouldFilterDecl` and fixed it in the same patch. ================ Comment at: clangd/index/SymbolCollector.cpp:74 + if (llvm::isa<TranslationUnitDecl>(Context) || + llvm::isa<LinkageSpecDecl>(Context)) + break; ---------------- ilya-biryukov wrote: > I may not know enough about the AST, sorry if the question is obvious. > `TranslationUnitDecl` is the root of the tree, but why should we stop at > `LinkageSpecDecl`? > > This code is probably going away per @hokein's comments. Symbols in `LinkageSpecDecl` (i.e. `extern "C"`) are C style symbols and do not have scopes. Also see my reply to Sam's related comment. ================ Comment at: clangd/index/SymbolCollector.cpp:195 llvm::SmallString<128> USR; + if (ND->getIdentifier() == nullptr) + return true; ---------------- ilya-biryukov wrote: > sammccall wrote: > > hokein wrote: > > > Consider moving to `shouldFilterDecl`? We also have a check `if > > > (ND->getDeclName().isEmpty())` there, which I assume does similar thing. > > hmm, what case is this handling? should `shouldFilterDecl` catch it? > Why do we skip names without identifiers? AFAIK, they are perfectly > reasonable C++ entities: overloaded operators, constructors, etc. `getName` crashes for `NamedDecl` without identifier. I thought symbols without identifier are not interesting for global code completion, so I added this filter to avoid crash. But on a second thought, these symbols would still be useful for go-to-definition, for example. This is no longer needed with `printQualifiedName` though. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D42796 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits