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

[Sam McCall via llvm-commits]

Right. And multiple TUs that *are* linked together would be fine too.
But in that case I don't think we need to be clever about treating these as
the same symbol. Indexing them twice is fine.

On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov <ibiryukov at google.com> wrote:

> In a single translation unit, yes. In multiple translation units that
> aren't linked together it's totally fine (may actually refer to different
> entities).
>
>
> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall <sammccall at google.com> wrote:
>
>> 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
>>>
>>>
>>>
>>>
>
> --
> Regards,
> Ilya Biryukov
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180202/1c69c955/attachment.html>