I was misreading: we set isIgnored if we're trying to generate a USR for a
linkagespecdecl itself (not a symbol in it).
For other e.g. a var, we check if the DC is a NamedDecl and if so, visit it
before visiting the var.
Linkagespec isn't a nameddecl, so this is a no-op. Result: things
(directly) under extern {} blocks don't get any outer scope info in their
USR. But not sure if this is intended (it's certainly not what *we* want!)
On Fri, Feb 2, 2018 at 2:05 PM, Ilya Biryukov <ibiryu...@google.com> wrote:
> At least now we know they might cause problems. Thanks for digging into
> this.
>
>
> On Fri, Feb 2, 2018 at 1:53 PM Sam McCall <sammcc...@google.com> wrote:
>
>> My intuition was that the USRs would be different, that linkage would
>> either be included or not included from the USR, but it wouldn't affect
>> whether the namespace is included. (Reasoning: USRs model language
>> concepts, not linker ones)
>>
>> But we're both wrong. If I'm reading USRGeneration correctly, hitting a
>> linkage spec while walking the scope tree sets the "ignore result" flag
>> which signals the result is unusable (and short-circuits some paths that
>> finish computing it). This seems like it may cause problems for us :-)
>> I wonder why the tests didn't catch it, maybe I'm misreading.
>>
>> On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov <ibiryu...@google.com>
>> wrote:
>>
>>> Exactly. We should make sure we *don't* treat them as the same symbol.
>>> But I would expect there USRs to be the same and that's what we use to
>>> deduplicate.
>>>
>>>
>>> On Fri, Feb 2, 2018 at 1:45 PM Sam McCall <sammcc...@google.com> wrote:
>>>
>>>> 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 <ibiryu...@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 <sammcc...@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 <
>>>>>> revi...@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
>>>>>
>>>>
>>>>
>>>
>>> --
>>> Regards,
>>> Ilya Biryukov
>>>
>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
