[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[Eric Liu via Phabricator via cfe-commits]

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