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.

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to