Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
Fixed prettyprinter in r324081 and USRs in r324093.
On Fri, Feb 2, 2018 at 2:16 PM, Sam McCall wrote:
> Talked to Ben, he thinks this is probably unintentional and that it's
> probably OK to change.
> I'll see if it breaks anything.
>
> On Fri, Feb 2, 2018 at 2:11 PM, Sam McCall wrote:
>
>> 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
>> 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 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
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
> 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
>> 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
>>> 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(Context) ||
> +llvm::isa(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 symbol
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
Talked to Ben, he thinks this is probably unintentional and that it's
probably OK to change.
I'll see if it breaks anything.
On Fri, Feb 2, 2018 at 2:11 PM, Sam McCall wrote:
> 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
> 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 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
>>> 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 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
> 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
>> 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(Context) ||
+llvm::isa(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.
>
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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 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 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
>> 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 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
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
> 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(Context) ||
>>> +llvm::isa(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
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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 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
> 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 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
>>> 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 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(Context) ||
>> +llvm::isa(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
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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 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 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
>> 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 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(Context) ||
> +llvm::isa(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
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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 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 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(Context) ||
>>> +llvm::isa(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
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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 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
> 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 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(Context) ||
+llvm::isa(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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 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(Context) ||
>> +llvm::isa(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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.
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(Context) ||
> +llvm::isa(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
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
