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

[Sam McCall via llvm-commits]

Yeah this is just a bug in clang's pprinter. I'll fix it.

If you give multiple C++ names to the same linker symbol using extern C,
I'm pretty sure you're in UB land.

On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
reviews at reviews.llvm.org> wrote:

> ilya-biryukov added inline comments.
>
>
> ================
> Comment at: clangd/index/SymbolCollector.cpp:73
> +       Context = Context->getParent()) {
> +    if (llvm::isa<TranslationUnitDecl>(Context) ||
> +        llvm::isa<LinkageSpecDecl>(Context))
> ----------------
> ioeric wrote:
> > 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.
> I think we want `X::y`, not `y`.
>
> Lookup still finds it inside the namespace and does not find it in the
> global scope. So for our purposes they are actually inside the namespace
> and have the qualified name of this namespace. Here's an example:
> ```
> namespace ns {
> extern "C" int foo();
> }
>
> void test() {
>   ns::foo(); // ok
>   foo(); // error
>   ::foo(); // error
> }
> ```
>
> Note, however, that the tricky bit there is probably merging of the
> symbols, as it means symbols with the same USR (they are the same for all
> `extern "c"` declarations with the same name, right?) can have different
> qualified names and we won't know which one to choose.
>
> ```
> namespace a {
>  extern "C" int foo();
> }
> namespace b {
>   extern "C" int foo(); // probably same USR, different qname. Also,
> possibly different types.
> }
> ```
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D42796
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180202/ff5da4e3/attachment.html>