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

[Ilya Biryukov via cfe-commits]

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/cfe-commits/attachments/20180202/73f6882d/attachment.html>