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

Ilya Biryukov via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 2 04:46:41 PST 2018


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 <sammccall at 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 <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
>>
>
>

-- 
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180202/14538f9c/attachment.html>


More information about the llvm-commits mailing list